-
Notifications
You must be signed in to change notification settings - Fork 44
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 CSRF token to GraphiQL view #35
base: master
Are you sure you want to change the base?
Conversation
Thanks for the PR! Can you add support for the other frameworks too (koa, hapi)? |
@tothandras I'll look at these other frameworks soon |
any updates on this? |
@@ -16,7 +16,8 @@ export function required() { | |||
|
|||
const GRAPHIQL_VERSION = '0.7.1'; | |||
|
|||
export function renderGraphiQL({ query, variables, version = GRAPHIQL_VERSION } = {}) { | |||
export function renderGraphiQL({ query, variables, version = GRAPHIQL_VERSION, csrfToken } = {}) { | |||
csrfToken = csrfToken ? `,'x-csrf-token': '${csrfToken}'` : ''; |
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.
It's usually a good idea to treat function parameters as immutable. In this case a better name could perhaps be csrfHeader
, as it's more aligned with the actual value.
@@ -89,7 +90,8 @@ export function renderGraphiQL({ query, variables, version = GRAPHIQL_VERSION } | |||
method: 'post', | |||
headers: { | |||
'Accept': 'application/json', | |||
'Content-Type': 'application/json', | |||
'Content-Type': 'application/json' |
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.
You can leave the trailing comma on this line. The main benefit of trailing commas is that they help reduce the size of diffs. This diff section could be +1
instead of +2 -1
.
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.
@CMTegner good points. I don't have time to see this through to completion, as I no longer use this library.
@phra sorry for the delay. I don't have time to work on this anymore, as I've moved on to using another library. Perhaps someone else is interested in taking this to the finish line by implementing @tothandras' request to add koa and hapi support? |
This change adds a
getCSRFToken
method that if provided can be invoked with therequest
to get a CSRF token for example:req.csrfToken()
.This is needed for securing the GraphiQL views (especially on production instances!). I'm using a modified version of this in development to allow my csrf strategy to work. Let me know what you think!