Add security and compatibility notes #303
Conversation
spec/GraphQLOverHTTP.md
Outdated
| 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. |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
+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?
There was a problem hiding this comment.
@jaydenseric @martinbonnin I took another pass at this paragraph. Please review the revised PR - thanks!
There was a problem hiding this comment.
@jaydenseric Any comments on the revised PR?
Co-authored-by: Jayden Seric <[email protected]>
spec/GraphQLOverHTTP.md
Outdated
| 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. |
There was a problem hiding this comment.
+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?
Co-authored-by: Martin Bonnin <[email protected]>
Co-authored-by: Martin Bonnin <[email protected]>
| 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 |
There was a problem hiding this comment.
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.
| 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.
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.
Based on the conversation within the 7-25-2024 working group, I've proposed a non-normative note on security and compatibility. Feel free to suggest changes to this text.