Revise non-normative notes section #345
Conversation
|
|
||
| Supporting formats not described by this specification, such as XML or Protobuf, |
There was a problem hiding this comment.
I removed these examples because I didn't want to imply that we might (or might not) be expanding to these formats in future. Clearer to exclude them IMO.
|
Merging this so I can look at merging #304 |
There was a problem hiding this comment.
Generally I like this PR. It lays down guidance for future changes to this RFC -- specifically, what this RFC does not cover. At first glance, most of the notes are under the subheading of Security, but honestly could be a generalized note with security being one aspect of it.
| For example, this specification allows alternative media types to be used to | ||
| encode the request body; however, media types such as `multipart/form-data` or | ||
| `application/x-www-form-urlencoded` may result in the request being treated by a | ||
| browser as a "simple request", which does not require a "preflight", thereby | ||
| opening the server up to Cross-Site Request Forgery (CSRF/XSRF) attacks. The | ||
| recommended `application/json` media type requires a "preflight" check when | ||
| issued cross-domain. See | ||
| [CORS protocol](https://fetch.spec.whatwg.org/#http-cors-protocol) in the WHATWG | ||
| Fetch spec for more details on this. |
There was a problem hiding this comment.
I like that this PR still retains explicit comments regarding the use of multipart/form-data, as this is not evident by the rest of the RFC and is important enough to worth of being explicitly noted, as it was/is a common practice.
| Note: One approach used by the community to mitigate CSRF risks is to ensure a | ||
| request is not "simple" by requiring a custom header—such as | ||
| `GraphQL-Require-Preflight`—is included. The presence of a custom header forces | ||
| browsers to enact a "preflight" check, thereby adding an additional layer of | ||
| security. (This is not a standard header, and many alternative headers could | ||
| serve the same purpose. This is presented merely as an example of a pattern seen | ||
| in the community.) |
There was a problem hiding this comment.
What about strengthening the suggestion of the header name to be used here, to help the community settle on a single header within servers that support multipart/form-data? While the above text is completely accurate, it's written as though GraphQL-Require-Preflight were only a random example, not necessarily the best choice. Giving guidance and suggestions within non-normative specifications does not seem to be contrary to its purpose, and would likely be appreciated by the community, in this case assisting implementations that wish to support multipart/form-data for various reasons (e.g. out-of-band file upload). Mentioning the header that Apollo uses (Apollo-Require-Preflight) may also be beneficial in this paragraph.
There was a problem hiding this comment.
I'm not convinced that requiring a preflight is a good security practice; the vast majority of GraphQL APIs IMO should either be closed off entirely (no cross-origin requests, first party only, trusted documents), or they should not support any credentials caching that a preflight would be relevant to (include all credentials in the request, none implicit). The <1% of use cases that fall outside of this should have their own experts who know what to do. Given that, I don't want us to be seen as standardizing one because it looks like we're also giving the approach legitimacy that I'm not sure it warrants. IMO it's a workaround to poor architectural choices or restrictions.
|
Thanks for the review @Shane32! 🙌 |
Follow up to:
I think it's important that we make sure that this section clearly sets out what is in or out of scope of the spec from a security point of view, whilst being very crisp on any reasoning added (for example, the reason that GET is not used for mutations is not purely due to security concerns).
This PR aims to add clarity to this section, restructuring it and expanding some areas, whilst contracting others. It aims to clearly demarcate examples, and surfaces many potential issues without offering many solutions, making it clear that these issues are truly out of scope (and non-exhaustive).
Further, because this is intended to be a non-normative section, it tries to be more factual: it provides statements of fact, and examples, rather than instructions in the form of "it is recommended that servers..." which imply normative levels.
(There's still one seemingly normative statement left in future compatibility, but I'm okay with that one.)
cc the people who reviewed or contributed to #303: @Shane32 @jaydenseric @martinbonnin @JoviDeCroock @alf