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.

Sign up for GitHub

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

Jump to bottom

Add security and compatibility notes #303

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add security and compatibility notes #303

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1940d1a
Add security and compatibility notes
Shane32 Jul 26, 2024
0e15911
Update formatting
Shane32 Jul 26, 2024
9d9bef9
Update GraphQLOverHTTP.md
Shane32 Jul 26, 2024
943596d
Update cspell.yml
Shane32 Jul 26, 2024
028e522
Merge branch 'patch-4' of https://github.com/Shane32/graphql-over-htt…
Shane32 Jul 26, 2024
68d0297
Update spec/GraphQLOverHTTP.md
Shane32 Jul 27, 2024
2a526be
Update spec/GraphQLOverHTTP.md
Shane32 Jul 27, 2024
0193d26
format
Shane32 Jul 27, 2024
52215cc
Update compatibility note
Shane32 Jul 27, 2024
c16c637
Update spec/GraphQLOverHTTP.md
Shane32 Jul 27, 2024
231338b
Update spec/GraphQLOverHTTP.md
Shane32 Jul 27, 2024
de9d7ad
Update spec/GraphQLOverHTTP.md
Shane32 Jul 27, 2024
befb96f
Reformat
Shane32 Jul 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cspell.yml
View file
Open in desktop
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
34 changes: 34 additions & 0 deletions spec/GraphQLOverHTTP.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,3 +745,37 @@ 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.

# Additional 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 "basic requests" under CORS (Cross-Origin Resource Sharing), meaning
Shane32 marked this conversation as resolved.
Show resolved Hide resolved
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.

Additionally, supporting form data requests (`application/x-www-form-urlencoded`
or `multipart/form-data`) could pose significant security risks. Form data
requests may be vulnerable to CSRF and other attacks due to the lack of CORS
preflight checks. As a result, the use of form data for GraphQL queries or
mutations is discouraged.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not keen on this wording; multipart/form-data can be done securely regarding CORS with the right headers enforced by the server. Lots of people successfully make GraphQL queries and mutations with GraphQL multipart requests in production systems.

The wording "is discouraged" is hostile to the GraphQL multipart request spec, which one day will be moved here into a GraphQL over HTTP spec.

A better approach would be to mention that "simple requests" don't have normal CORS behavior, and to give a few examples of the kinds of requests that are "simple". multipart/form-data shouldn't be singled out as somehow worse than any other kind of simple request. In my spec, I don't prescribe what people should do to achieve security, I just note that they should be aware that they need to understand the implications and account for it:

https://github.com/jaydenseric/graphql-multipart-request-spec?tab=readme-ov-file#security

Most people just enforce in their GraphQL server middleware that if the request is multipart/form-data, a certain custom header must be present, indicating that the request isn't "simple".

Copy link
Contributor

@martinbonnin martinbonnin Jul 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1.

FWIW, Apollo has Apollo-Require-Preflight (doc).

I'm not overly familiar with the topic but I would welcome an explicit (non-normative) recommendation in this note to help implementers. GraphQL-Require-Preflight maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaydenseric @martinbonnin I took another pass at this paragraph. Please review the revised PR - thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaydenseric Any comments on the revised PR?


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.

## Format Compatibility

Supporting formats not described by this specification, such as XML or Protobuf,
may conflict 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.
Shane32 marked this conversation as resolved.
Show resolved Hide resolved
Loading