Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

馃摎 Update docs with Helmet CSP support #701

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

a-tokyo
Copy link

@a-tokyo a-tokyo commented Oct 12, 2020

  • Adds documentation explaining how to use Graphiql with the common express middleware Helmet.

Related issue: apollographql/apollo-server#4648

@a-tokyo
Copy link
Author

a-tokyo commented Oct 12, 2020

Will reopen after handling all edge cases

@a-tokyo a-tokyo closed this Oct 12, 2020
@a-tokyo a-tokyo reopened this Oct 12, 2020
/** @by-us - adds graphiql support over helmet's default CSP */
"'unsafe-inline'",
/** @by-us - adds graphiql support over helmet's default CSP */
"'unsafe-eval'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need eval here?
The same goes for inline?
Maybe it's something we can address in GraphiQL?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IvanGoncharov For some reason chrome threw errors saying smth like "script violates security policy, please add unsafe-eval"

You can reproduce easily by removing these 2 lines from an express/graphiql server and opening graphiql -- a blank screen shows, and the above errors are logged

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to fix our code to be CSP compliant in the next release.
It looks like unsafe-eval is required by GraphiQL you can help by submitting an issue there so the GraphiQL team can start working on it.

Copy link
Member

@acao acao Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably has to do with our unpkg example and the script tag attributes we use? are we using cdn assets in this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure it's not caused by this?
https://github.com/graphql/express-graphql/blob/master/resources/load-statically-from-npm.js

I do not see this issue with chrome when visiting https://graphiql-test.netlify.com

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acao It's probably because you not setting CSP for graphiql-test.netlify.com
Can you please try it with this snippet https://github.com/graphql/express-graphql/pull/701/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R359 ?

Base automatically changed from master to main February 10, 2021 15:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants