Enforce content type for POST requests #689
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tgwizard
force-pushed
the
enforce-request-content-type
branch
from
4349786
to
f7d0b9c
Compare
Hmm, Maybe I've actually broken this in the rebase. #655 adds support for multipart forms. Can I get a sanity check before I cut a release?
Lets make it very clear in the release notes. Between this and the complexity changes we probably should flag this as 0.9, they both have a decent chance to ruin someones day. |
|
Oh, sorry, I didn't realize this was merged. The changes look good to me after the rebase too. |
cgxxv
pushed a commit
to cgxxv/gqlgen
that referenced
this pull request
Mar 25, 2022
…t-type Enforce content type for POST requests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the gqlgen HTTP handler doesn't look at the request
Content-Typeheader, but just assumes that the request is JSON.This opens up for a CSRF vulnerability. Servers that accept only
application/jsonusually don't need to care about CSRF if they've disabled CORS, since HTML forms on other pages cannot submit data withapplication/jsoncontent type. However, currently, a HTML form on another site can submit atext/plainrequest but with a valid JSON body, and the gqlgen server will happily accept it. If the service does auth through cookies, this can be dangerous.This PR validates the request
Content-Typeheader, similarly to howexpress-graphqldoes it: https://github.com/graphql/express-graphql/blob/1eeaa141b5419834c11872741d9c5d12cfa227c3/src/parseBody.js#L51-L59.express-graphqlsupportsapplication/x-www-form-urlencodedandapplication/graphqltoo though, which we could if we wanted.This PR shouldn't break well-behaving clients, but it definitely could. We could add a config option
AllowedContentTypes, where the server could specify which content types it allows. The default (nil/empty slice) could be to support all content types, maintaining backwards compatibility but continuing the slightly weird behaviour, or the default could be to allow "all supported content types", which could mean theapplication/x-www-form-urlencodedandapplication/graphqlin the future too.What do you think?
I have: