Improve Security of GraphiQL #508

danpalmer · 2018-08-30T18:13:16Z

This PR makes the following security improvements to GraphiQL in graphene-django.

Stores the GraphQL query in the URL fragment, rather than the query, so that sensitive data won't be logged to web server logs.
Moves JS to a separate file to enable stricter Content Security Policies.

graingert · 2018-08-30T18:14:38Z

graphene_django/templates/graphene/graphiql.html

@@ -24,41 +24,13 @@
 </head>
 <body>
  <script>
+


Strange whitespace change?

This was me "boyscouting" to make the code more readable, but I decided it was unnecessary when I re-did these changes.

coveralls · 2018-08-30T18:17:13Z

Coverage decreased (-0.3%) to 94.405% when pulling 2b08e59 on danpalmer:graphiql-no-querystring into 9351626 on graphql-python:master.

graingert · 2018-08-30T18:21:36Z

Might be better to fill the query into the editor, and only execute it when the button is pressed.

A further security measure could be to use a warning dialogue for queries loaded from the URL when the submit button is pressed?

graingert · 2018-08-30T18:22:19Z

What about using a fragment instead?

danpalmer · 2018-08-30T18:27:54Z

Might be better to fill the query into the editor, and only execute it when the button is pressed.

A further security measure could be to use a warning dialogue for queries loaded from the URL when the submit button is pressed?

I think these are orthogonal concerns, but I will look into doing them.

What about using a fragment instead?

Good call, I will do this.

We can also now return GraphiQL earlier in the request handling.

danpalmer · 2018-08-30T18:50:24Z

@graingert can you re-review please. I think this implements everything you suggested, if I understood correctly. Also special cased browser refreshes so that you don't get prompted to allow the query if you're just refreshing the page (which I frequently do to refresh the schema).

graingert · 2018-08-30T18:53:26Z

graphene_django/templates/graphene/graphiql.html

@@ -41,7 +41,7 @@
    });
    // Produce a Location query string from a parameter object.


This comment isn't quite right anymore

graingert · 2018-08-30T19:15:40Z

graphene_django/templates/graphene/graphiql.html

+    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?")) {


Might be worth including an explanation of why running an untrusted query might be a bad idea, and what damage it could cause.

Also something like "if you're not sure click cancel"

This is the Facebook devtools warning for eg:

Stop! This is a browser feature intended for developers. If someone told you to copy and paste something here to enable a Facebook feature or "hack" someone's account, it is a scam and will give them access to your Facebook account. See https://www.facebook.com/selfxss for more information.

graingert · 2018-08-30T19:20:43Z

Looks like handling untrusted input on submission will need changes in Graphiql

This also allows the introspection query through so that the user can edit with intellisense before being prompted.

danpalmer · 2018-08-30T19:33:05Z

@graingert I'm not sure it will. I've updated with my implementation, although I'm not sure that it's perfect.

adamchainz · 2018-08-30T19:50:23Z

graphene_django/templates/graphene/graphiql.html

-      document.body
-    );
-  </script>
+  <script src="{% static "graphene_django/graphiql.js" %}"></script>


Doesn't this imply graphene_django relies on staticfiles now?

Yes. Do you think that needs documenting? I don’t know how optional it is in Django.

graphene_django/templates/graphene/graphiql.html

danpalmer · 2018-08-30T20:00:45Z

@syrusakbary could you take a look at this and let me know what you think?

For what it's worth, the main issue - GraphQL queries being in the URL query-string has bitten us and we'll probably have to disable GraphiQL if we can't find a way to improve the security.

syrusakbary · 2018-08-31T10:23:18Z

Hey @danpalmer, thanks for the PR.

Thanks to the way the GraphQL view is architected, mutations will not be executed if the request is coming through a GET request (meaning: someone accessing an URL from the browser).
See code here: https://github.com/graphql-python/graphene-django/blob/master/graphene_django/views.py#L260-L263
However, query operations will be executed regardless (for both GET and POST HTTP verbs).

I think, rather than disabling eager GraphQL execution at all times, we can just have a flag like:

graphiql_eager_execution = True / False

That will indicate to execute the GraphQL query in GraphiQL eagerly or not.
We could make it True by default to not break old behaviour.

The code for the GraphQL view / GraphiQL integration is highly inspired by how graphql-express works. Demo of GraphiQL here: https://graphql.org/swapi-graphql/
That's why I'm a bit hesitant to go out of the norm for GraphQL/GraphiQL views, since all frameworks we have adopted a similar approach.

Thoughts?

danpalmer · 2018-08-31T11:12:28Z

@syrusakbary

Thanks to the way the GraphQL view is architected, mutations will not be executed if the request is coming through a GET request (meaning: someone accessing an URL from the browser).

Yeah I noticed this, it's a good decision. How about we always eagerly execute queries, and prompt the user for mutations. I think it's worth being clear about the untrusted nature of the mutations.

The code for the GraphQL view / GraphiQL integration is highly inspired by how graphql-express works. That's why I'm a bit hesitant to go out of the norm for GraphQL/GraphiQL views, since all frameworks we have adopted a similar approach.

Thanks for this, I'm not familiar with other implementations. I agree that we should try to work in a similar way, although I think the change to using a fragment rather than the query string to store the query is an important security feature - so a place where I think it's worth breaking away from their design.

we'll probably have to disable GraphiQL if we can't find a way to improve the security.

Also, apologies for this, I received feedback that it could be read as much more hostile than I intended. The query string change is necessary for us to start exposing GraphiQL to other developers internally, so I'm happy to make whatever changes are necessary for this to be merged.

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.

danpalmer · 2018-09-09T20:46:08Z

@syrusakbary thanks for all the feedback on this.

I've backed out the changes to query execution in GraphiQL for now as I think they need more thought, or just for the change to be made in GraphiQL itself.

Updated the PR description with the features this now implements. Would be great to get this reviewed and merged!

syrusakbary · 2018-09-09T20:50:41Z

Great, will take a time to review it practically tomorrow :)

jplock · 2018-09-09T21:32:59Z

This PR will make
#308 more difficult to implement. Is there anyway to combine them?

danpalmer · 2018-09-09T21:43:24Z

@jplock ah interesting, I don't think this should make it any more difficult to implement #308 – there will be a very minor merge conflict, but there are review points that need addressing that will cause a rewrite of those lines anyway, so I think I'm going to leave the changes for that PR.

danpalmer · 2018-09-18T09:46:26Z

Hey, quick ping on this. I believe all feedback has been addressed and that it's ready for merge.

danpalmer · 2018-09-22T16:56:19Z

@syrusakbary any update on this?

danpalmer · 2018-09-28T08:56:39Z

I believe this is ready to merge and that I've addressed all review points. If there's something outstanding here please let me know – I'm happy to address! Would be nice to get this merged soon though.

danpalmer · 2018-09-28T08:57:24Z

@jkimbo are you able to take a look and merge?

jkimbo

@danpalmer unfortunately I don't have write access so you'll have to wait on @syrusakbary

Code looks good though 👍

danpalmer · 2018-10-11T17:16:39Z

@syrusakbary ping on this. It's been hanging around for a while and I don't want it to bit rot. Also we're scaling up the number of devs working with GraphQL, so we would appreciate the security enhancements here (notably not logging the queries in webserver logs).

danpalmer · 2018-10-24T11:40:21Z

@syrusakbary @patrick91 @grantmcconnaughey @pizzapanther @spockNinja @RadoRado

Paging contributors. Apologies for the mentions but we've tried pinging and bumping this and haven't get anywhere so far.

It would be a real shame for graphene-django to bitrot and stop getting updates. We're investing heavily in it and would love to contribute, but PRs taking months to get merged discourages us from doing so.

This PR is good to go, it's been reviewed, all criticism so far has been addressed (sufficiently I believe). We're more than happy to fix any additional code review points, but otherwise it would be great to get clarity on why this isn't being merged, especially as it's addressing a security vulnerability.

patrick91 · 2018-10-24T12:04:13Z

I can approve but I don't have merge permissions, unfortunately

danpalmer · 2018-10-24T12:06:17Z

It seems like we have quite high bus factor on this repo. Who has merge permissions? Can they be extended to other frequent contributors like @patrick91 or @jkimbo?

danpalmer · 2018-10-24T12:46:30Z

@patrick91 it looks like you do have merge permissions? GitHub requires "1 approving review by reviewers with write access", and it's listing you under that.

pizzapanther · 2018-10-24T12:50:47Z

Even if you have GitHub permission what about pypi to publish it?

danpalmer · 2018-10-24T13:03:21Z

@pizzapanther merging would at least prevent this PR from bitrotting and would allow people to target the master branch in their requirements if they really wanted to, so I think it would be an improvement, but yes PyPI access would be ideal.

patrick91 · 2018-10-24T14:00:13Z

@danpalmer this is what I get, even when there's other reviews from members of the org

I can push to the repo, but not on master :)

danpalmer · 2018-10-24T14:02:52Z

Ah damn, sounds like GitHub PR review requirements and protected branches aren't fully compatible. Thanks for checking though!

danpalmer · 2018-12-04T18:33:05Z

@syrusakbary Ping on this. Would be good to talk about the process for PRs getting merged and releases getting shipped in the long run, it would be a real shame for this package to be unmaintained.

syrusakbary · 2018-12-04T18:36:31Z

Hi @danpalmer, sorry for the long delay here.
I'm now very immersed in a new venture and takes almost all my time.

I'm working on laying out a plan for Graphene so things like this can be reviewed and merged without my explicit approval, and the project can continue growing at a healthy pace.

pizzapanther · 2018-12-04T18:45:06Z

We should call the person who did flatmap-stream on npm. Think he likes to take over projects. 🤣

patrick91 · 2018-12-04T20:35:18Z

Hi @danpalmer, sorry for the long delay here.
I'm now very immersed in a new venture and takes almost all my time.

I'm working on laying out a plan for Graphene so things like this can be reviewed and merged without my explicit approval, and the project can continue growing at a healthy pace.

finally!

syrusakbary · 2018-12-27T13:49:50Z

Ok, I'm starting to roll the plan.

Please take a look on the main issue in Graphene to provide input and feedback into how you think Graphene can be governed in the long term :)

graphql-python/graphene#884

adamchainz · 2019-03-16T11:57:10Z

@jkimbo thanks!

graingert reviewed Aug 30, 2018

View reviewed changes

danpalmer added 4 commits August 30, 2018 19:29

Don't execute on GET for GraphiQL

2190059

We can also now return GraphiQL earlier in the request handling.

Use the fragment for the URL

3755850

Pass options from the fragment, not the template context

0d8f9db

Special case reloads as allowed if we can

9a5b355

danpalmer force-pushed the graphiql-no-querystring branch from 58e7986 to 9a5b355 Compare August 30, 2018 18:48

danpalmer changed the title ~~Don't put queries in the querystring~~ Improve Security of GraphiQL Aug 30, 2018

graingert approved these changes Aug 30, 2018

View reviewed changes

danpalmer added 2 commits August 30, 2018 20:31

Allow the user to see the query before prompting

d1b734f

This also allows the introspection query through so that the user can edit with intellisense before being prompted.

Fix comment

24ebc20

Move GraphiQL's JS into a separate file for ease of CSP

e50e12b

adamchainz reviewed Aug 30, 2018

View reviewed changes

graingert reviewed Aug 30, 2018

View reviewed changes

graphene_django/templates/graphene/graphiql.html Outdated Show resolved Hide resolved

danpalmer added 2 commits August 30, 2018 20:58

Change quotes to improve some syntax highlighting

7e8f6db

Document that staticfiles is now a dependency.

cb87f40

Revert to default query execution behaviour

2b08e59

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.

jkimbo approved these changes Oct 3, 2018

View reviewed changes

patrick91 approved these changes Oct 24, 2018

View reviewed changes

patrick91 mentioned this pull request Dec 6, 2018

Is this still being maintained? #561

Closed

jkimbo merged commit 297b807 into graphql-python:master Mar 16, 2019

shoof1072 mentioned this pull request Jun 20, 2019

404 Not Found after upgrade to 2.3 #685

Closed

		@@ -41,7 +41,7 @@
		});
		// Produce a Location query string from a parameter object.

Improve Security of GraphiQL #508

Improve Security of GraphiQL #508

Conversation

danpalmer commented Aug 30, 2018 • edited Loading

graingert Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

danpalmer Aug 30, 2018

Choose a reason for hiding this comment

coveralls commented Aug 30, 2018 • edited Loading

graingert commented Aug 30, 2018

graingert commented Aug 30, 2018

danpalmer commented Aug 30, 2018

danpalmer commented Aug 30, 2018

graingert Aug 30, 2018

Choose a reason for hiding this comment

graingert Aug 30, 2018

Choose a reason for hiding this comment

graingert commented Aug 30, 2018

danpalmer commented Aug 30, 2018

adamchainz Aug 30, 2018

Choose a reason for hiding this comment

danpalmer Aug 30, 2018

Choose a reason for hiding this comment

danpalmer Aug 30, 2018

Choose a reason for hiding this comment

danpalmer commented Aug 30, 2018

syrusakbary commented Aug 31, 2018 • edited Loading

danpalmer commented Aug 31, 2018

danpalmer commented Sep 9, 2018

syrusakbary commented Sep 9, 2018

jplock commented Sep 9, 2018

danpalmer commented Sep 9, 2018

danpalmer commented Sep 18, 2018

danpalmer commented Sep 22, 2018

danpalmer commented Sep 28, 2018

danpalmer commented Sep 28, 2018

jkimbo left a comment

Choose a reason for hiding this comment

danpalmer commented Oct 11, 2018

danpalmer commented Oct 24, 2018

patrick91 commented Oct 24, 2018

danpalmer commented Oct 24, 2018

danpalmer commented Oct 24, 2018

pizzapanther commented Oct 24, 2018

danpalmer commented Oct 24, 2018

patrick91 commented Oct 24, 2018

danpalmer commented Oct 24, 2018

danpalmer commented Dec 4, 2018

syrusakbary commented Dec 4, 2018

pizzapanther commented Dec 4, 2018

patrick91 commented Dec 4, 2018

syrusakbary commented Dec 27, 2018

adamchainz commented Mar 16, 2019

danpalmer commented Aug 30, 2018 •

edited

Loading

graingert Aug 30, 2018 •

edited

Loading

coveralls commented Aug 30, 2018 •

edited

Loading

syrusakbary commented Aug 31, 2018 •

edited

Loading