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

Should we explicitly support Content-Type: application/graphql? #249

Open
benjie opened this issue Apr 11, 2023 · 13 comments
Open

Should we explicitly support Content-Type: application/graphql? #249

benjie opened this issue Apr 11, 2023 · 13 comments
Labels

Comments

@benjie
Copy link
Member

benjie commented Apr 11, 2023

Reviewing https://github.com/graphql/graphql.github.io/pull/1394/files I noticed that servers were recommended to accept application/graphql where the POST body would be a raw (text) GraphQL document - indeed PostGraphile has always supported this. It doesn't actually detail handling variables in this case, which is why I excluded it from the spec, but we could handle them via query parameters in the same way as we do for GET.

Question: should we explicitly include this in the spec. The BestPractice-ServingOverHTTP.md page "recommends" it, so if we did add it, it would be a SHOULD (not a MUST).

I'm personally of the opinion that we should just use the JSON encoding and not add Content-Type: application/graphql support (servers MAY still support it, but that's their choice) - I don't think it's a good idea to add too many different ways to do the same thing, it complicates compatibility for everyone. But I feel it warrants a discussion none the less.

Thoughts, @abernix @balshor @benjie @deinok @ErikWittern @jaydenseric @michaelstaib @mike-marcacci @mmatsa @Shane32 @sjparsons @spawnia @sungam3r @Touchstone117 @enisdenjo @JoviDeCroock ?

@enisdenjo
Copy link
Member

I share Benjies opinion, an extra content-type that'd have a questionable approach to defining variables (and even not an optimal one due to URL size limits) would be a maintenance burden IMHO.

I think the spec should recommend the most ideal of approaches and be kept simple and concise.

@spawnia
Copy link
Member

spawnia commented Apr 11, 2023

Is this approach actually used significantly? I tried to search GitHub for mentions in issues with the following query: https://github.com/search?q=%22application%2Fgraphql%22&type=issues. I found it hard to make sense of the result. A couple of issues seem to request or mention the usage of application/graphql proper, others are false-positives, e.g. application/graphql+json.

@Shane32
Copy link
Contributor

Shane32 commented Apr 11, 2023

I agree (i.e. don't require support), although considering that existing servers (including GraphQL.NET Server) support application/graphql, it may be worthwhile to publish a brief appendix that outlines the suggested support level for that media type when enabled. It could likely be summed up into a single paragraph such as the following:

Servers MAY additionally support the application/graphql media type for POST requests, but such support is not encouraged. When supported, the POST body content contains the query parameter of the GraphQL request verbatim. Character encoding SHOULD be assumed to be utf-8 unless specified otherwise within the HTTP headers. The operation name, variables, and/or extensions GraphQL request parameters MAY be supported via query string parameters in the same manner as a GET request.

Here are some additional comments regarding application/graphql support:

  • It may be convenient for command-line tools to use; probably more so for servers who do not enable GET requests than those that do
  • Variable support, as well as operation-name support, would likely end up on the query string, as it is with GET requests (or would not be supported).
  • Supporting variables over the query string could be considered a security issue if personal data is passed within a variable, as it is more likely to be logged in plaintext within the HTTP infrastructure. Of course, any request via GET suffers the same symptom.

@ghmcadams
Copy link
Contributor

When we decided to remove application/graphql in favor of application/graphql-json (which later became application/graphql-response+json), we did so because it covered all use cases without being problematic for any. I think that it is better to hold ground and stick with what we have.

The spec allows for application/json because most/all servers use it today. If it is the case that newer implementations areusing application/graphql (perhaps due to the earlier versions of this spec), then they should adjust to use the current spec recommendations (which have been available for a while now).

@Touchstone117
Copy link
Contributor

I think I agree that it shouldn't be specified, having multiple ways of doing things in the spec will lead to fragmentation in preferred implementations.

@benjie
Copy link
Member Author

benjie commented Apr 12, 2023

Great; seems like consensus. I’m going to leave this open as an opportunity for someone to add the appendix on “historical approaches” or whatever. When doing so you may wish to note the application/x-www-form-urlencoded and multipart/form-data approaches have also both been used in the past by various clients/servers, but that they have security implications (see @glasser’s writings on this subject) and therefore they are NOT RECOMMENDED.

@jaydenseric
Copy link
Contributor

@benjie

When doing so you may wish to note the application/x-www-form-urlencoded and multipart/form-data approaches have also both been used in the past by various clients/servers, but that they have security implications (see @glasser’s writings on this subject) and therefore they are NOT RECOMMENDED.

multipart/form-data when implemented correctly is secure, and is very popular in the wild because it solves a common problem in an intuitive way. It's not in any way a deprecated technique. In fact, at work right now we're using it in a brand new enterprise GraphQL API we're building that will have very stringent security audits. There is an open issue here to ratify it: #7 .

@glasser
Copy link
Contributor

glasser commented Apr 12, 2023

This seems like a tangent on this issue, but: it seems unlikely that anyone (or at least anyone who uses cookies) will implement it the multipart/form-data-based protocol if you don't merge my 10-month old PR adding a security note to the spec.

@benjie
Copy link
Member Author

benjie commented Apr 12, 2023

Indeed, I didn't say they couldn't be implemented securely, just that they have security implications - I don't think it's worth our effort to dedicate resources to addressing these in the GraphQL-over-HTTP spec when the graphql-multipart-request-spec already exists to address those concerns. Perhaps calling the appendix "Other content types" would be wise, and it can link out to spec(s) but make it clear it's not an endorsement (since the working group probably doesn't have sufficient resource to vet it).

@mmatsa
Copy link
Contributor

mmatsa commented Apr 16, 2023

I'm happy with the consensus on this issue for the same reason as everyone else - not specifying multiple ways to do things in a spec.

To the question from @spawnia :

Is this approach actually used significantly?

I remember about 3 years ago being in a graphql-wg meeting when that question came up and someone (I think @eapache Evan from Shopify but I'm not sure) said that based on their logs from all the many clients that hit their public endpoint about half of the transactions used application/graphql. I don't remember if it was 30%, 40%, or what, but I remember that everyone on the zoom, including me, was surprised.

I've tried just now to search the web for the meeting notes that would have the actual number and see whether it confirms my memory or not --- but I haven't found it. Maybe @benjie has a better way to search the meeting notes.

@eapache
Copy link
Member

eapache commented Apr 16, 2023

This rings a vague bell for me, but I don’t remember concretely, and I’m unfortunately no longer at Shopify to check. I believe @swalkinshaw or @rebeccajfriedman can probably confirm this though?

@benjie
Copy link
Member Author

benjie commented Apr 17, 2023

@swalkinshaw
Copy link

Shopify had some simple "getting started" examples from ~2017 to 2019 that used application/graphql which is probably what led to higher usage. It's definitely caused a lot of confusion over the years (example) and I'm in favour of using the MAY language for it.

Now we see >99% of requests using application/json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests