Skip to content

feat(mergeAST): add mergeAST utility #4359

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

Open
wants to merge 1 commit into
base: 16.x.x
Choose a base branch
from

Conversation

metrue
Copy link

@metrue metrue commented Mar 22, 2025

This PR is adding utility of merge AST by simply migrating it from graphiql

As first step to resolve this issue #1428 (comment)

@metrue metrue requested a review from a team as a code owner March 22, 2025 19:50
@metrue metrue changed the title WIP: feat(mergeAST): add mergeAST util feat(mergeAST): add mergeAST util Mar 23, 2025
@metrue metrue changed the title feat(mergeAST): add mergeAST util feat(mergeAST): add mergeAST utility Mar 23, 2025
Copy link

github-actions bot commented Apr 3, 2025

@github-actions publish-pr-on-npm

@0042834736 The latest changes of this PR are available on NPM as
graphql@16.10.0-canary.pr.4359.9fe5229b2fc30d3e5b07a6b00693fac42b649fdd
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-4359

@benjie
Copy link
Member

benjie commented Apr 10, 2025

One thing we should very much keep in mind with mergeAST is that it's a potential DOS vector. Having it in the frontend (i.e. GraphiQL) is one thing, but putting it in GraphQL.js means that we will need to have a lot stronger protection against things that could cause explosion of complexity or even infinite loops. If we could require that the document was already validated (and thus had passed all our checks) before calling into mergeAST then that would give us some confidence, but I imagine that people will want to mergeAST separately and we need to protect them from bad inputs. I'm not sure the solution to this, just wanted to surface the concern.

@metrue
Copy link
Author

metrue commented Jun 9, 2025

Thanks @benjie .

You're absolutely right: mergeAST introduces complexity risks, especially if used on unvalidated or hostile input. My intention with duplicated the utility from GraphiQL is to support use cases like: dynamically composing selection sets, merging query documents for internal middleware, or some other client side use cases, or Building tooling that operates on statically constructed or pre-validated ASTs. But yeah it might introduce reliability and security risks in server-side GraphQL environments.

Given that context, I'd be happy to explore ways to make the function safer or more clearly scoped, I am thinking,

  • We can rename it to mergeValidatedAST, explicitly require/assume that inputs to mergeAST are the result of parse() followed by validate()
  • Add shallow complexity safeguards (depth, cost)

It might not be able to prevent all the malicious input I think. Would love your thoughts on whether any of these sound like a reasonable direction. I'm happy to iterate and contribute safeguards in this PR or a follow-up depending on what fits best with the project’s expectations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants