Skip to content

Add shouldPersistHeaders = true to GraphiQL options #257

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

Closed
wants to merge 1 commit into from
Closed

Add shouldPersistHeaders = true to GraphiQL options #257

wants to merge 1 commit into from

Conversation

marceloverdijk
Copy link
Contributor

This PR comes with a caveat. Question is if this option should be set to true by default because of security reasons (this is also the reason why it is not enabled by default).

This is also related to #136 enabling the GraphiQL header editor which is already merged to provide e.g. authorization headers like an API key.

This is also my use case, my GraphQL server requires an API key which I can provide via headers editor and it works as expected, thus so far so good.

But without the shouldPersistHeaders = true option headers are not persisted so on reload (or just opening GraphiQL page) the headers editor is empty.
Of course no problem to add the header again manually, but the consequence is that when GraphiQL is loaded it will fire an initial request to load the Schema which is used for displaying the docs and auto-completion...
As headers are currently not stored that first request to get the schema details fails and no documentation and auto-completion available unfortunately, making the GraphiQL interface far from optimal...

As I said it is questionable if this value should be set by default.
Alternative could be to have some spring.graphql.graphiql.options or spring.graphql.graphiql.additional-options property to add custom options to the GraphiQL interface. Something like:

spring:
  graphql:
    graphiql:
       options:
         headerEditorEnabled: true
         shouldPersistHeaders: true

Footnote: I wonder only having headerEditorEnabled = true and not shouldPersistHeaders is really useful, as I think most will use it for authorization, and that is needed on GraphQL request to get schema as well.

@koenpunt
Copy link
Contributor

and that is needed on GraphQL request to get schema as well.

This depends on how authorization is implemented; our graphql api is public, but has fields that require authorization, so retrieving the schema for introspection works without.

That being said, I do think that persisting the headers is a valuable option, and I also think that most implementations will use (or should) temporary/short lived tokens, so personally I don't see a security issue in having it enabled by default.

@bclozel bclozel added this to the 1.0.0 milestone Apr 21, 2022
@bclozel bclozel self-assigned this Apr 21, 2022
@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 21, 2022
@bclozel bclozel closed this in d8c2bab May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants