Skip to content

Query Limiting Depth #1472

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
AstRonin opened this issue Oct 24, 2023 · 5 comments · Fixed by #1475
Closed

Query Limiting Depth #1472

AstRonin opened this issue Oct 24, 2023 · 5 comments · Fixed by #1475

Comments

@AstRonin
Copy link

AstRonin commented Oct 24, 2023

Is your feature request related to a problem? Please describe.
Graphene has query validation about Depth limit Validator (https://docs.graphene-python.org/en/latest/execution/queryvalidation/#depth-limit-validator).

validation_errors = validate(
    schema=schema.graphql_schema,
    document_ast=parse('THE QUERY'),
    rules=(
        depth_limit_validator(
            max_depth=20
        ),
    )
)

Would be great we will have it (depth) in django project.

Describe the solution you'd like
Limit depth of query. OWASP suggest add limit for security reason https://cheatsheetseries.owasp.org/cheatsheets/GraphQL_Cheat_Sheet.html#query-limiting-depth-amount

@AstRonin
Copy link
Author

Can this
#1342 (comment)
example usable also for my question?

@kiendang
Copy link
Collaborator

@AstRonin I believe that would work.

@firaskafri @sjdemartini @ulgens This use case has been requested a couple of time. If we go the route of porting over graphql-core graphql_impl to improve performance (see #1439 (comment) and #1393 (comment)), this would be quite easy to add validation_rules as another option to GraphQLView. I could submit a PR for this, but currently I'm not using it so defer to you on it's worth implementing or not.

@sjdemartini
Copy link
Collaborator

@kiendang I like the idea of adding validation rules as an option to GraphQLView. I didn't realize that was already a feature of graphene (and just not graphene-django). Sounds like it's worthwhile given requests like this and the ability to support both graphene's built-in validation like depth-limits, as well as potentially other user-defined validators.

This was referenced Oct 26, 2023
@sjdemartini
Copy link
Collaborator

@kiendang now that you've merged in the performance improvements and refactoring around graphene-django's query execution (thank you!), are you still planning on adding an option for validation rules in the GraphQLView? No rush, just curious. I've recently been thinking more about validation in a GraphQL app I work on, and this seems like it'd be a nice addition to graphene-django to enable some of that.

@kiendang
Copy link
Collaborator

kiendang commented Nov 1, 2023

Yup I just put up a PR in #1475. Implementation is simple but I haven't added any test though.

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

Successfully merging a pull request may close this issue.

3 participants