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

feat: support passing options to graphql validate and parse #1050

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

markrzen
Copy link
Contributor

@markrzen markrzen commented Nov 8, 2023

Support passing options to GraphQL's validate and parse functions.

Reasoning to allow configuration of these functions topics is fine tuning to prevent DDOS attacks.

This allows developers to set:

  1. validate's options of maxErrors to something more/less than the 100 default. This prevents an attacker from specifying 100 invalid fields, requiring CPU time generating validation errors.
  2. parse's options of maxTokens to a value. It has no default. This prevent an attacker from producing a overly high complexity query that requires a high amount of CPU time.

@markrzen markrzen marked this pull request as ready for review November 9, 2023 15:50
@markrzen
Copy link
Contributor Author

@mcollina Can we get this on a reviewers queue? I wouldn't usually poke maintainers like this, but previous PRs seem to be getting reviewed within a few days. I wanted to make sure this didn't fall through the cracks.

@markrzen
Copy link
Contributor Author

@simoneb Any chance this is something you can review?

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines 38 to 39
- `parseOptions`: Object. [GraphQL's parse function options](https://github.com/graphql/graphql-js/blob/main/src/language/parser.ts#L77-L133)
- `validateOptions`: Object. [GraphQL's validate function options](https://github.com/graphql/graphql-js/blob/main/src/validation/validate.ts#L44)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would avoid deep links to source code, as it can change any time. The alternative would be to use permalinks, but I'm not sure that's a good option either. Is there official documentation that we can point to instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The published graphql docs do not contain option properties sadly. I could update them to a specific tag if that works for you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

not many compelling options then. I'm not sure. if we point at master, those links may refer to wrong lines when those files change. if we use permalinks, the reference will keep existing but the real code might have changed. not sure what's best really. shall we link to the files without any line number perhaps? that would give us some confidence that those files will keep existing and leave it to the reader to figure out where to find the types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could just enumerate the literal properties, rather than linking off. It does mean our future selfs will need to go back to the docs and confirm when things have been added/removed.

Not sure there is a good option, other than pushing for these functions to be documented more fully in GraphQL library.

Happy to change to without line numbers if that makes anyone feel more comfortable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No line numbers is the compromise I'd go with, but happy for somebody else to chime in too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! :)

index.js Show resolved Hide resolved
Copy link
Collaborator

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this!

@simoneb simoneb merged commit 65a9ec6 into mercurius-js:master Nov 15, 2023
10 checks passed
@markrzen
Copy link
Contributor Author

@simoneb @mcollina Thanks y'all. What sort of release schedule are y'all doing? Any expectations I should have?

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.

None yet

3 participants