application/json should only mandate 200 for _well-formed_ GraphQL-over-HTTP requests

[benjie]

Reviewing @enisdenjo's GraphQL-over-HTTP audits, I realise that I made a mistake when drafting this specification. In my head, each time I wrote "request" I meant "GraphQL-over-HTTP request", i.e. one that roughly conforms to:

type GraphQLOverHTTPRequest = {
  query: string;
  operationName?: string | null;
  variables?: Record<string, any> | null;
  extensions?: Record<string, any> | null;
}

However this was not explicitly stated in enough places, the end result being that we now effectively state that if you send garbage at a GraphQL legacy server it should return a 200 back at you. This was not my intent - my intent that 200 should only be used if the request conformed to the TypeScript shape above - query doesn't have to be parseable, and doesn't have to validate against your schema, but it does have to be present and it does have to be a string. If you can't even parse the JSON body of a request, you should definitely be returning a 400 Bad Request not a 200 Okay.

I've performed some edits to make this clearer, and have also copied the examples from application/graphql-response+json to application/json and corrected them for that location. I've also moved a few notes around where they would have been repeated otherwise, and made a few other small edits in line with accomplishing the above.

cc @graphql/http-maintainers

[spawnia]

Good change overall

[enisdenjo]

This is great, often GQL servers are in situations where they don't do the parsing themselves (fastify or express does it for you) and therefore have limited control.

Also related to graphql/graphql-http#47.

[benjie]

@Shane32 I've updated the text you commented on and added a Note: - do you think this adds clarity, or the Note: should be omitted?

[Shane32]

@Shane32 I've updated the text you commented on and added a Note: - do you think this adds clarity, or the Note: should be omitted?

I don’t mind the extra clarity. Personally, it’s second nature to me to always ensure query arguments and sql parameters are correctly encoded ever since the days of Classic ASP, but judging from the amount of sql injection attacks over the years, a little extra clarity here is probably a good thing.

FYI, another option is to revert the change, and instead add a note stating that parameters are not double encoded but were restated for clarity.

[benjie]

@graphql/http-maintainers Does anyone want me to hold off on merging this?

[Shane32]

Looks good. I just noted a typographical changes you may wish to make. Feel free to dismiss if not applicable.

[enisdenjo]

Changes to the audit suite are already merged in graphql-http graphql/graphql-http#62.