Skip to content
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

Separate Property for GraphiQL Path #70

Closed
rstoyanchev opened this issue Jun 28, 2021 · 6 comments
Closed

Separate Property for GraphiQL Path #70

rstoyanchev opened this issue Jun 28, 2021 · 6 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@rstoyanchev
Copy link
Contributor

By default we use the same "/graphql" path to serve the GraphiQL page with HTTP GET, GraphQL queries over HTTP POST, as well as subscriptions over WebSocket. There is a websocket property in GraphQlProperties but the path property is used for GraphiQL and for GraphQL queries.

We should allow GraphiQL to use a different path so that the DGS framework can build on that. Should we also consider using a different path by default? I like the idea to make it more clearly a different URL that is secured differently, as planned with #38.

@rstoyanchev rstoyanchev added this to the 1.0 M1 milestone Jun 28, 2021
@bclozel
Copy link
Member

bclozel commented Jun 28, 2021

I'm quite happy with the current situation which I think makes things simpler for the security configuration: developers only have to worry about securing a single URL for access to the graphQL API in general, possibly refining required roles at the data fetching level.

But I'm fine with a separate property for the path to the GraphiQL UI if it helps DGS in any way.

Also, I don't see the pros about changing that value by default as:

  • we don't support GET requests for graphQL calls (same for DGS as far as I understand), so that spot in the request mapping space is wasted otherwise.
  • the GraphiQL UI is useless on its own - unless the goal is to give users a UI they can use for a third party graphQL endpoint. In that case, building your own GraphiQL instance might be a better choice.
  • As stated by Andi in Secure Configuration for GraphiQL and Introspection #38, I don't understand why introspection is even discussed. Assuming one has access to the API, disabling such mechanisms is security by obscurity.

I'd rather make our security configuration as simple as possible to avoid user errors, for example securing the GraphiQL UI but not the actual endpoint, or getting the HTTP verbs wrong.

@rstoyanchev
Copy link
Contributor Author

Thanks, Brian.

Indeed we don't support queries with HTTP GET, but the convention exists, and I was thinking that if a client tries to send a query with GET, it would be more clear to return a 405 with an "Allow: POST" header, instead of coming back with the GraphiQL index page.

It could work better for security authorization purposes too, to clearly disambiguate the URL path for GraphiQL and have it protected out of the box.

As stated by Andi in Secure Configuration for GraphiQL and Introspection #38, I don't understand why introspection is even discussed. Assuming one has access to the API, disabling such mechanisms is security by obscurity.

It was a mistake to create one issue for both. Do we agree at least that securing the GraphiQL page out of the box is a good idea? The point about introspection, I don't believe is without merit, but that can be a separate discussion.

@paulbakker
Copy link

For context, the reason we chose /graphiql in DGS in the past was:

  1. There are multiple alternatives to graphiql (although things are consolidating a bit). By using /graphiql we don't need to choose the one editor to rule them all. Each can live on its own path.
  2. Although we still don't support GET requests, it is something that some folks do for better caching. There's a good talk from the New York Times about this. By not taking the /graphql path, we keep that option open for the future.
  3. It seems to be what most folks expect coming from other Graphql environments.

None of these reasons are extremely convincing, but that's how we got to that choice.

@andimarek
Copy link
Contributor

I think it is debatable what the default value should be, but I agree that it should be a separate property.

The main reason is 2. from @paulbakker.

We want people to implement/allow GET requests in the future imho.

@rstoyanchev
Copy link
Contributor Author

Keeping the option open for the future is a good reason. From that perspective, it also means changing the default value in order to keep that option open, and to be able to return a 405 in the mean time to indicate clearly that HTTP GET is not supported for queries.

In retrospect, overloading the same (resource) path for two very different things doesn't seem such a good idea, even if convenient, and the point about multiple alternatives graphiql alternatives further underscores the need to separate more.

@rstoyanchev
Copy link
Contributor Author

I've added the 405 response for HTTP GET on the query path.

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

No branches or pull requests

4 participants