From 219005952a1d143fce6363be3d4d3bc8f8d0ecc5 Mon Sep 17 00:00:00 2001 From: Dan Palmer Date: Thu, 30 Aug 2018 19:29:33 +0100 Subject: [PATCH 01/10] Don't execute on GET for GraphiQL We can also now return GraphiQL earlier in the request handling. --- graphene_django/views.py | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/graphene_django/views.py b/graphene_django/views.py index be7ccf96c..9a530de93 100644 --- a/graphene_django/views.py +++ b/graphene_django/views.py @@ -124,6 +124,12 @@ def dispatch(self, request, *args, **kwargs): data = self.parse_body(request) show_graphiql = self.graphiql and self.can_display_graphiql(request, data) + if show_graphiql: + return self.render_graphiql( + request, + graphiql_version=self.graphiql_version, + ) + if self.batch: responses = [self.get_response(request, entry) for entry in data] result = "[{}]".format( @@ -137,19 +143,6 @@ def dispatch(self, request, *args, **kwargs): else: result, status_code = self.get_response(request, data, show_graphiql) - if show_graphiql: - query, variables, operation_name, id = self.get_graphql_params( - request, data - ) - return self.render_graphiql( - request, - graphiql_version=self.graphiql_version, - query=query or "", - variables=json.dumps(variables) or "", - operation_name=operation_name or "", - result=result or "", - ) - return HttpResponse( status=status_code, content=result, content_type="application/json" ) From 3755850c2e5de11eb80f7d39d0f8fd31f3cc5d66 Mon Sep 17 00:00:00 2001 From: Dan Palmer Date: Thu, 30 Aug 2018 19:47:48 +0100 Subject: [PATCH 02/10] Use the fragment for the URL --- graphene_django/templates/graphene/graphiql.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/graphene_django/templates/graphene/graphiql.html b/graphene_django/templates/graphene/graphiql.html index 1ba0613b3..5bc5e04a5 100644 --- a/graphene_django/templates/graphene/graphiql.html +++ b/graphene_django/templates/graphene/graphiql.html @@ -32,7 +32,7 @@ // Collect the URL parameters var parameters = {}; - window.location.search.substr(1).split('&').forEach(function (entry) { + window.location.hash.substr(1).split('&').forEach(function (entry) { var eq = entry.indexOf('='); if (eq >= 0) { parameters[decodeURIComponent(entry.slice(0, eq))] = @@ -41,7 +41,7 @@ }); // Produce a Location query string from a parameter object. function locationQuery(params) { - return '?' + Object.keys(params).map(function (key) { + return '#' + Object.keys(params).map(function (key) { return encodeURIComponent(key) + '=' + encodeURIComponent(params[key]); }).join('&'); From 0d8f9db3fbeef93be194d386f87dea627f69715e Mon Sep 17 00:00:00 2001 From: Dan Palmer Date: Thu, 30 Aug 2018 19:48:21 +0100 Subject: [PATCH 03/10] Pass options from the fragment, not the template context --- .../templates/graphene/graphiql.html | 31 +++++++++++-------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/graphene_django/templates/graphene/graphiql.html b/graphene_django/templates/graphene/graphiql.html index 5bc5e04a5..6515da828 100644 --- a/graphene_django/templates/graphene/graphiql.html +++ b/graphene_django/templates/graphene/graphiql.html @@ -100,22 +100,27 @@ function updateURL() { history.replaceState(null, null, locationQuery(parameters)); } - // Render into the body. - ReactDOM.render( - React.createElement(GraphiQL, { - fetcher: graphQLFetcher, + // If there are any fragment parameters, confirm the user wants to use them. + if (Object.keys(parameters).length + && !window.confirm("An untrusted query has been loaded, continue loading query?")) { + parameters = {}; + } + var options = { + fetcher: graphQLFetcher, onEditQuery: onEditQuery, onEditVariables: onEditVariables, onEditOperationName: onEditOperationName, - query: '{{ query|escapejs }}', - response: '{{ result|escapejs }}', - {% if variables %} - variables: '{{ variables|escapejs }}', - {% endif %} - {% if operation_name %} - operationName: '{{ operation_name|escapejs }}', - {% endif %} - }), + query: parameters.query, + } + if (parameters.variables) { + options.variables = parameters.variables; + } + if (parameters.operation_name) { + options.operationName = parameters.operation_name; + } + // Render into the body. + ReactDOM.render( + React.createElement(GraphiQL, options), document.body ); From 9a5b3556d3d13ec3c46ea4bc953f1df844f0925d Mon Sep 17 00:00:00 2001 From: Dan Palmer Date: Thu, 30 Aug 2018 19:48:38 +0100 Subject: [PATCH 04/10] Special case reloads as allowed if we can --- graphene_django/templates/graphene/graphiql.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/graphene_django/templates/graphene/graphiql.html b/graphene_django/templates/graphene/graphiql.html index 6515da828..0303883d7 100644 --- a/graphene_django/templates/graphene/graphiql.html +++ b/graphene_django/templates/graphene/graphiql.html @@ -101,7 +101,9 @@ history.replaceState(null, null, locationQuery(parameters)); } // If there are any fragment parameters, confirm the user wants to use them. + var isReload = window.performance ? performance.navigation.type === 1 : false; if (Object.keys(parameters).length + && !isReload && !window.confirm("An untrusted query has been loaded, continue loading query?")) { parameters = {}; } From d1b734f07df87f97f3acc557d990952e3a250e7d Mon Sep 17 00:00:00 2001 From: Dan Palmer Date: Thu, 30 Aug 2018 20:31:39 +0100 Subject: [PATCH 05/10] Allow the user to see the query before prompting This also allows the introspection query through so that the user can edit with intellisense before being prompted. --- .../templates/graphene/graphiql.html | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/graphene_django/templates/graphene/graphiql.html b/graphene_django/templates/graphene/graphiql.html index 0303883d7..de3126a9d 100644 --- a/graphene_django/templates/graphene/graphiql.html +++ b/graphene_django/templates/graphene/graphiql.html @@ -58,9 +58,31 @@ otherParams[k] = parameters[k]; } } + + // If there are any fragment parameters, confirm the user wants to use them. + var isReload = window.performance ? performance.navigation.type === 1 : false; + var isQueryTrusted = Object.keys(parameters).length === 0 || isReload; + var fetchURL = locationQuery(otherParams); + // Defines a GraphQL fetcher using the fetch API. function graphQLFetcher(graphQLParams) { + var isIntrospectionQuery = ( + graphQLParams.query !== parameters.query + && graphQLParams.query.indexOf('IntrospectionQuery') !== -1 + ); + + if (!isQueryTrusted + && !isIntrospectionQuery + && !window.confirm("This query was loaded from a link, are you sure you want to execute it?")) { + return Promise.resolve('Aborting query.'); + } + + // We don't want to set this for the introspection query + if (!isIntrospectionQuery) { + isQueryTrusted = true; + } + var headers = { 'Accept': 'application/json', 'Content-Type': 'application/json' @@ -100,13 +122,6 @@ function updateURL() { history.replaceState(null, null, locationQuery(parameters)); } - // If there are any fragment parameters, confirm the user wants to use them. - var isReload = window.performance ? performance.navigation.type === 1 : false; - if (Object.keys(parameters).length - && !isReload - && !window.confirm("An untrusted query has been loaded, continue loading query?")) { - parameters = {}; - } var options = { fetcher: graphQLFetcher, onEditQuery: onEditQuery, From 24ebc20bf449b4688220e2dac0b43240d0fc5a6c Mon Sep 17 00:00:00 2001 From: Dan Palmer Date: Thu, 30 Aug 2018 20:32:38 +0100 Subject: [PATCH 06/10] Fix comment --- graphene_django/templates/graphene/graphiql.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphene_django/templates/graphene/graphiql.html b/graphene_django/templates/graphene/graphiql.html index de3126a9d..cf61686de 100644 --- a/graphene_django/templates/graphene/graphiql.html +++ b/graphene_django/templates/graphene/graphiql.html @@ -39,7 +39,7 @@ decodeURIComponent(entry.slice(eq + 1)); } }); - // Produce a Location query string from a parameter object. + // Produce a Location fragment string from a parameter object. function locationQuery(params) { return '#' + Object.keys(params).map(function (key) { return encodeURIComponent(key) + '=' + From e50e12bc9fa693bde1af1b61f2a97eaba90007bd Mon Sep 17 00:00:00 2001 From: Dan Palmer Date: Thu, 30 Aug 2018 20:36:26 +0100 Subject: [PATCH 07/10] Move GraphiQL's JS into a separate file for ease of CSP --- .../static/graphene_django/graphiql.js | 119 +++++++++++++++++ .../templates/graphene/graphiql.html | 120 +----------------- 2 files changed, 121 insertions(+), 118 deletions(-) create mode 100644 graphene_django/static/graphene_django/graphiql.js diff --git a/graphene_django/static/graphene_django/graphiql.js b/graphene_django/static/graphene_django/graphiql.js new file mode 100644 index 000000000..ad55e0340 --- /dev/null +++ b/graphene_django/static/graphene_django/graphiql.js @@ -0,0 +1,119 @@ +(function() { + + // Parse the cookie value for a CSRF token + var csrftoken; + var cookies = ('; ' + document.cookie).split('; csrftoken='); + if (cookies.length == 2) + csrftoken = cookies.pop().split(';').shift(); + + // Collect the URL parameters + var parameters = {}; + window.location.hash.substr(1).split('&').forEach(function (entry) { + var eq = entry.indexOf('='); + if (eq >= 0) { + parameters[decodeURIComponent(entry.slice(0, eq))] = + decodeURIComponent(entry.slice(eq + 1)); + } + }); + // Produce a Location fragment string from a parameter object. + function locationQuery(params) { + return '#' + Object.keys(params).map(function (key) { + return encodeURIComponent(key) + '=' + + encodeURIComponent(params[key]); + }).join('&'); + } + // Derive a fetch URL from the current URL, sans the GraphQL parameters. + var graphqlParamNames = { + query: true, + variables: true, + operationName: true + }; + var otherParams = {}; + for (var k in parameters) { + if (parameters.hasOwnProperty(k) && graphqlParamNames[k] !== true) { + otherParams[k] = parameters[k]; + } + } + + // If there are any fragment parameters, confirm the user wants to use them. + var isReload = window.performance ? performance.navigation.type === 1 : false; + var isQueryTrusted = Object.keys(parameters).length === 0 || isReload; + + var fetchURL = locationQuery(otherParams); + + // Defines a GraphQL fetcher using the fetch API. + function graphQLFetcher(graphQLParams) { + var isIntrospectionQuery = ( + graphQLParams.query !== parameters.query + && graphQLParams.query.indexOf('IntrospectionQuery') !== -1 + ); + + if (!isQueryTrusted + && !isIntrospectionQuery + && !window.confirm("This query was loaded from a link, are you sure you want to execute it?")) { + return Promise.resolve('Aborting query.'); + } + + // We don't want to set this for the introspection query + if (!isIntrospectionQuery) { + isQueryTrusted = true; + } + + var headers = { + 'Accept': 'application/json', + 'Content-Type': 'application/json' + }; + if (csrftoken) { + headers['X-CSRFToken'] = csrftoken; + } + return fetch(fetchURL, { + method: 'post', + headers: headers, + body: JSON.stringify(graphQLParams), + credentials: 'include', + }).then(function (response) { + return response.text(); + }).then(function (responseBody) { + try { + return JSON.parse(responseBody); + } catch (error) { + return responseBody; + } + }); + } + // When the query and variables string is edited, update the URL bar so + // that it can be easily shared. + function onEditQuery(newQuery) { + parameters.query = newQuery; + updateURL(); + } + function onEditVariables(newVariables) { + parameters.variables = newVariables; + updateURL(); + } + function onEditOperationName(newOperationName) { + parameters.operationName = newOperationName; + updateURL(); + } + function updateURL() { + history.replaceState(null, null, locationQuery(parameters)); + } + var options = { + fetcher: graphQLFetcher, + onEditQuery: onEditQuery, + onEditVariables: onEditVariables, + onEditOperationName: onEditOperationName, + query: parameters.query, + } + if (parameters.variables) { + options.variables = parameters.variables; + } + if (parameters.operation_name) { + options.operationName = parameters.operation_name; + } + // Render into the body. + ReactDOM.render( + React.createElement(GraphiQL, options), + document.body + ); +})(); diff --git a/graphene_django/templates/graphene/graphiql.html b/graphene_django/templates/graphene/graphiql.html index cf61686de..7bd1178f2 100644 --- a/graphene_django/templates/graphene/graphiql.html +++ b/graphene_django/templates/graphene/graphiql.html @@ -5,6 +5,7 @@ If you wish to receive JSON, provide the header "Accept: application/json" or add "&raw" to the end of the URL within a browser. --> +{% load static %} @@ -23,123 +24,6 @@ - + From 7e8f6dbd4ec0fe633507e5ba6bc22bda4a0e59fc Mon Sep 17 00:00:00 2001 From: Dan Palmer Date: Thu, 30 Aug 2018 20:58:00 +0100 Subject: [PATCH 08/10] Change quotes to improve some syntax highlighting --- graphene_django/templates/graphene/graphiql.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphene_django/templates/graphene/graphiql.html b/graphene_django/templates/graphene/graphiql.html index 7bd1178f2..af1127484 100644 --- a/graphene_django/templates/graphene/graphiql.html +++ b/graphene_django/templates/graphene/graphiql.html @@ -24,6 +24,6 @@ - + From cb87f4016546aac5f4973e6d01fb31b0cbc10d4e Mon Sep 17 00:00:00 2001 From: Dan Palmer Date: Thu, 30 Aug 2018 20:59:09 +0100 Subject: [PATCH 09/10] Document that staticfiles is now a dependency. --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 4e0b01d4d..ef3f40cf0 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,7 @@ pip install "graphene-django>=2.0" ```python INSTALLED_APPS = ( # ... + 'django.contrib.staticfiles', # Required for GraphiQL 'graphene_django', ) From 2b08e59bea2a0d84dc68832dc6d2deae98e77d3a Mon Sep 17 00:00:00 2001 From: Dan Palmer Date: Sun, 9 Sep 2018 21:44:30 +0100 Subject: [PATCH 10/10] Revert to default query execution behaviour The only security risk here is persuading a user to execute a mutation, which is probably not a big risk. To mitigate this risk and still keep the same UX (that is so valuable), would require more work than is proportionate for this PR. --- .../static/graphene_django/graphiql.js | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/graphene_django/static/graphene_django/graphiql.js b/graphene_django/static/graphene_django/graphiql.js index ad55e0340..2be7e3c8f 100644 --- a/graphene_django/static/graphene_django/graphiql.js +++ b/graphene_django/static/graphene_django/graphiql.js @@ -35,30 +35,10 @@ } } - // If there are any fragment parameters, confirm the user wants to use them. - var isReload = window.performance ? performance.navigation.type === 1 : false; - var isQueryTrusted = Object.keys(parameters).length === 0 || isReload; - var fetchURL = locationQuery(otherParams); // Defines a GraphQL fetcher using the fetch API. function graphQLFetcher(graphQLParams) { - var isIntrospectionQuery = ( - graphQLParams.query !== parameters.query - && graphQLParams.query.indexOf('IntrospectionQuery') !== -1 - ); - - if (!isQueryTrusted - && !isIntrospectionQuery - && !window.confirm("This query was loaded from a link, are you sure you want to execute it?")) { - return Promise.resolve('Aborting query.'); - } - - // We don't want to set this for the introspection query - if (!isIntrospectionQuery) { - isQueryTrusted = true; - } - var headers = { 'Accept': 'application/json', 'Content-Type': 'application/json'