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

Add security and compatibility notes #303

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions cspell.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ words:
# Software
- ical
- WebDAV
- Protobuf
# Deliberate typos
- qeury
- __typena
Expand Down
42 changes: 42 additions & 0 deletions spec/GraphQLOverHTTP.md
Original file line number Diff line number Diff line change
Expand Up @@ -745,3 +745,45 @@ 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
Shane32 marked this conversation as resolved.
Show resolved Hide resolved

## 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
Copy link

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:

For requests that are more involved than what is possible with HTML’s form element, a CORS-preflight request is performed, to ensure request’s current URL supports the CORS protocol.

https://fetch.spec.whatwg.org/#http-cors-protocol

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
Copy link

Choose a reason for hiding this comment

The 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 GraphQL-Require-Preflight.

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. While it is recommended to primarily
adhere to the officially recognized formats outlined here, experimentation with
other encodings is encouraged to explore their potential benefits.
Shane32 marked this conversation as resolved.
Show resolved Hide resolved
Loading