-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add security and compatibility notes #303
Open
Shane32
wants to merge
13
commits into
graphql:main
Choose a base branch
from
Shane32:patch-4
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
1940d1a
Add security and compatibility notes
Shane32 0e15911
Update formatting
Shane32 9d9bef9
Update GraphQLOverHTTP.md
Shane32 943596d
Update cspell.yml
Shane32 028e522
Merge branch 'patch-4' of https://github.com/Shane32/graphql-over-htt…
Shane32 68d0297
Update spec/GraphQLOverHTTP.md
Shane32 2a526be
Update spec/GraphQLOverHTTP.md
Shane32 0193d26
format
Shane32 52215cc
Update compatibility note
Shane32 c16c637
Update spec/GraphQLOverHTTP.md
Shane32 231338b
Update spec/GraphQLOverHTTP.md
Shane32 de9d7ad
Update spec/GraphQLOverHTTP.md
Shane32 befb96f
Reformat
Shane32 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ words: | |
# Software | ||
- ical | ||
- WebDAV | ||
- Protobuf | ||
# Deliberate typos | ||
- qeury | ||
- __typena | ||
|
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -745,3 +745,44 @@ payload is `application/json` then the client MUST NOT rely on the body to be a | |
well-formed _GraphQL response_ since the source of the response may not be the | ||
server but instead some intermediary such as API gateways, proxies, firewalls, | ||
etc. | ||
|
||
# Non-normative notes | ||
|
||
## Security | ||
|
||
In this specification, GET requests are not supported for mutations due to | ||
security concerns. GET requests expose variables to logging mechanisms and | ||
intermediaries due to the URL encoding of parameters, which can lead to | ||
sensitive data being inadvertently logged. Furthermore, GET requests are | ||
considered "simple requests" under CORS (Cross-Origin Resource Sharing), meaning | ||
they bypass preflight checks that add a layer of security. | ||
|
||
On the other hand, using `application/json` for request bodies mandates a CORS | ||
preflight request, adding a security layer by ensuring the client has explicit | ||
permission from the server before sending the actual request. This is | ||
particularly important in mitigating cross-site request forgery (CSRF) attacks. | ||
|
||
It's important to note that "simple requests" like those using | ||
`application/x-www-form-urlencoded` or `multipart/form-data` do not have the | ||
same CORS behavior, and thus do not undergo the same preflight checks. | ||
Implementers should be aware of the security implications of using these types | ||
of requests. While they can be secured with the right headers enforced by the | ||
server, it is crucial to understand and properly account for the security risks | ||
involved. | ||
|
||
To mitigate these risks, it is recommended that servers require a custom header | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about: To mitigate these risks, it is recommended that servers require a custom header. Since this is not possible using forms, a preflight check must occur. While the name and value of the header is not important, we suggest |
||
to ensure requests are not "simple." For instance, a `GraphQL-Require-Preflight` | ||
header can be used to indicate that a preflight check has occurred, providing an | ||
additional layer of security. | ||
Shane32 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
For more detailed security considerations, please refer to | ||
[RFC 7231](https://tools.ietf.org/html/rfc7231), | ||
[RFC 6454](https://tools.ietf.org/html/rfc6454), and other relevant RFCs. | ||
|
||
## Request format compatibility | ||
|
||
Supporting formats not described by this specification, such as XML or Protobuf, | ||
may have potential conflicts with future versions of this specification as | ||
ongoing development aims to standardize and ensure the security and | ||
interoperability of GraphQL over HTTP. For this reason, it is recommended to | ||
adhere to the officially recognized formats outlined here. |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very nice addition.
The term “simple request” is used but not defined and I can’t find the definition in any of the references either. How about defining it using the following quote from the fetch spec? I believe that’s the authoritative source:
https://fetch.spec.whatwg.org/#http-cors-protocol