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

RFC: Support Persisted Queries for Mutations #1287

Closed
juanallo opened this issue Jan 8, 2021 · 10 comments · Fixed by #2951
Closed

RFC: Support Persisted Queries for Mutations #1287

juanallo opened this issue Jan 8, 2021 · 10 comments · Fixed by #2951
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release needs more info ✋ A question or report that needs more info to be addressable

Comments

@juanallo
Copy link

juanallo commented Jan 8, 2021

Summary

Currently the @urql/exchange-persisted-fetch only supports queries. Enabling persisted queries for mutations has a couple of advantages:

Security

One of the challenges of GQL is that an API is exposed for arbitrary requests. The flexibility in the query language allows bad actors to try DDoS attacks by generating big expensive queries. By leveraging persisted queries at build time you can effectively limit the combinations and defend against these types of attacks. 

https://leapgraph.com/graphql-api-security#persist-queries

Efficiency

While queries are usually larger than mutations there is still small improvements in not sending that information in the request. Here flexibility is the enabler, in use cases where mutations for business reasons have to be big in size persisted mutations could be a solution to reduce latency.

Apollo-client compatibility

Apollo-client supports persisted queries for mutations and providing the same feature would help developers that are thinking to port projects into URQL.

Proposed Solution

Enable mutations in the Persisted Fetch Exchange. It could be added as an opt in feature to provide backwards compatibility to projects that don’t need mutations to be hashed. 

@juanallo juanallo added the future 🔮 An enhancement or feature proposal that will be addressed after the next release label Jan 8, 2021
@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Jan 8, 2021

Hey,

I don't think Apollo supports this currently, code. This checks whether or not it contains any mutation reference and if it does it goes to regular querying.
This is btw also the case for persistGraphQL

It feels a bit dangerous for caching reasons to use GET for something that is intended for POST purposes. I also think most server implementations won't support this.

@juanallo
Copy link
Author

juanallo commented Jan 8, 2021

Agree I don't think GET should be a valid case for hashed mutations, but I do think there is value in allowing POST mutations to be sent using a hash.

Which Apollo supports currrently.

@kitten
Copy link
Member

kitten commented Feb 2, 2021

@juanallo Hiya 👋 I haven't found any code or documentation that details this. I suppose we could send something like persisted queries against a POST request with a body that's similar to persisted queries. However, I haven't found any hints of this being supported by servers like the Apollo Server implementation. Can you point out where you've maybe seen this as being supported? I haven't found any specification from Apollo's end for this yet anyway, so I'm unsure of what the details of the implementation should be.

@kitten kitten added the needs more info ✋ A question or report that needs more info to be addressable label Feb 2, 2021
@kitten
Copy link
Member

kitten commented Feb 17, 2021

Closing due to inactivity

@stalniy
Copy link

stalniy commented Jun 7, 2021

I’m using this approach currently with Apollo client and Apollo server using there is built in persisted query mechanism with custom queries cache.

All works like a charm.

But now we are moving to urql and we need this functionality for mutations.

@stalniy
Copy link

stalniy commented Jun 7, 2021

There is no any reason to not implement it because it highly improves security of graphql api.

Even if it’s not in any spec or standard.

I think this should be reopened and implemented. I can even send a PR

@kitten
Copy link
Member

kitten commented Jun 8, 2021

@stalniy It's definitely something we can support and something we should support, however, we do need to have guarantees that we're not implementing a random feature here, but rather a feature / mechanism that a majority of GraphQL APIs that aim to support APQ/PQs will buy into. Hence, we were asking for references to implementations.

This could include references to code, references to this being implemented in more than one GraphQL API, etc. I see this as a little tricky because both file uploads and persisted queries are currently following looser specs that are not comprehensively documented just yet. So, the issue I'm trying to carefully talk about here is that if every implementation of Apollo's APQ/PQ goes by observed effects, loose documents, and some pieces of code, all implementations may arrive at different tiny quirks.

Anyway, what we can do is enable mutations to be sent with the same mechanism as POST-only always. But I'd love to be able to see first whether this is actually expected in all implementations of APQ that exist, e.g. Mercurius: https://mercurius.dev/#/docs/persisted-queries

@stalniy
Copy link

stalniy commented Jun 8, 2021

Maybe if I rephrase this to add filter option to filter out operations based on some conditions. Would it make more sense?

Frankly speaking, I don't want to spend my time investigating other implementations/docs, I just want to have an extension point, to persist mutations and queries because this extremely improves security of my api. Maybe nobody support this currently but I or you would start to write a spec about this, everybody would start to do this or maybe not. It depends on how important security for a particular domain/customer.

Making mutations to be persisted brings a lot of benefits but if this benefits does not weight enough to add additional option, that's fine. I will just create a custom exchange.

@rriski
Copy link

rriski commented Jun 28, 2021

I also ran into this transitioning from apollo. I'm using Postgraphile with @graphile/persisted-operations. I'm generating a hash for all of my client mutations and queries at built time with graphql-codegen-persisted-query-ids and using that list of hashes as an allowlist on the server. Now I have to disable persisted queries for mutations since urql doesn't support them. So seconding the calls for implementing persisted queries for mutations.

@geekuillaume
Copy link
Contributor

Restarting this conversation. I'm in the same situation as @rriski. Adding an optional filter option to configure when this exchange is used would be perfect. This logic is already used by Apollo client which also apply the persisted operation logic on mutations (https://github.com/apollographql/apollo-client/blob/main/src/link/persisted-queries/index.ts). Apollo client also force using POST for mutations.

It should be noted that in this usecase, persisted operations are used to create an allow-list of operation in the backend from the queries extracted from a frontend app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release needs more info ✋ A question or report that needs more info to be addressable
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants