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

fix: Add graphql max depth and aliases limits #955

Merged
merged 10 commits into from
Nov 6, 2024
4 changes: 4 additions & 0 deletions codecov/settings_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,10 @@

GRAPHQL_INTROSPECTION_ENABLED = False

GRAPHQL_MAX_DEPTH = 15
Copy link
Contributor

Choose a reason for hiding this comment

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

Wanted to hear a little more about landing on 15 for both of these values, do you think stage/production/dev should have the same value for each?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I skimmed the existing queries and the max depth was around ~10, so I thought it would be a good buffer. Could potentially do like 20 if we want more wiggle room. I think keeping it the same in all envs makes sense since honestly I think it's just a set-and-forget kind of thing. Also, we are the main consumers of the graphql api so I'd want to catch any queries that would error in prod to be caught first in the lower envs at the same settings.

For aliases, I don't think we ever use more than a handful (<5?) at a time, so thought a reasonable use case wouldn't go beyond say 15 anyway (and anything above that seems to be a malicious actor).

Open to thoughts on those. If any become a problem, it's as simple as bumping this up & we would catch the issue during development of some new feature anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally makes sense! I agree with you having it ~15 for the depth makes a lot of sense. Could probably lower the alias one if we really wanted, though not a deal breaker


GRAPHQL_MAX_ALIASES = 15

# Database
# https://docs.djangoproject.com/en/2.1/ref/settings/#databases

Expand Down
100 changes: 100 additions & 0 deletions graphql_api/tests/test_validation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
from graphql import (
GraphQLField,
GraphQLObjectType,
GraphQLSchema,
GraphQLString,
parse,
validate,
)

from ..validation import (
create_max_aliases_rule,
create_max_depth_rule,
)


def resolve_field(*args):
return "test"


QueryType = GraphQLObjectType(
"Query", {"field": GraphQLField(GraphQLString, resolve=resolve_field)}
)
schema = GraphQLSchema(query=QueryType)


def validate_query(query, *rules):
ast = parse(query)
return validate(schema, ast, rules=rules)


def test_max_depth_rule_allows_within_depth():
query = """
query {
field
}
"""
errors = validate_query(query, create_max_depth_rule(2))
assert not errors, "Expected no errors for depth within the limit"


def test_max_depth_rule_rejects_exceeding_depth():
query = """
query {
field {
field {
field
}
}
}
"""
errors = validate_query(query, create_max_depth_rule(2))
assert errors, "Expected errors for exceeding depth limit"
assert any(
"Query depth exceeds the maximum allowed depth" in str(e) for e in errors
)


def test_max_depth_rule_exact_depth():
query = """
query {
field
}
"""
errors = validate_query(query, create_max_depth_rule(2))
assert not errors, "Expected no errors when query depth matches the limit"


def test_max_aliases_rule_allows_within_alias_limit():
query = """
query {
alias1: field
alias2: field
}
"""
errors = validate_query(query, create_max_aliases_rule(2))
assert not errors, "Expected no errors for alias count within the limit"


def test_max_aliases_rule_rejects_exceeding_alias_limit():
query = """
query {
alias1: field
alias2: field
alias3: field
}
"""
errors = validate_query(query, create_max_aliases_rule(2))
assert errors, "Expected errors for exceeding alias limit"
assert any("Query uses too many aliases" in str(e) for e in errors)


def test_max_aliases_rule_exact_alias_limit():
query = """
query {
alias1: field
alias2: field
}
"""
errors = validate_query(query, create_max_aliases_rule(2))
assert not errors, "Expected no errors when alias count matches the limit"
65 changes: 65 additions & 0 deletions graphql_api/validation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
from typing import Any, Type

from graphql import GraphQLError, ValidationRule
from graphql.language.ast import DocumentNode, FieldNode, OperationDefinitionNode
from graphql.validation import ValidationContext


def create_max_depth_rule(max_depth: int) -> Type[ValidationRule]:
class MaxDepthRule(ValidationRule):
def __init__(self, context: ValidationContext) -> None:
super().__init__(context)
self.operation_depth: int = 1
self.max_depth_reached: bool = False
self.max_depth: int = max_depth

def enter_operation_definition(
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, are these functions base functions you had to override default behavior of?

Similar story with enter_field, leave_field, and enter_document

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Though I don't think about it so much as "override" as "implement the interface" that is defined here. The function names enter_X and leave_X are dynamic per here. And for any method that's not implemented explicitly, it behaves as a no-op (here).

this is the stuff I looked at in Ariadne doc & this example implementation

Copy link
Contributor

Choose a reason for hiding this comment

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

Very cool! Thanks for linking all those docs, that was a fun set of reads

self, node: OperationDefinitionNode, *_args: Any
) -> None:
self.operation_depth = 1
self.max_depth_reached = False

def enter_field(self, node: FieldNode, *_args: Any) -> None:
self.operation_depth += 1

if self.operation_depth > self.max_depth and not self.max_depth_reached:
self.max_depth_reached = True
self.report_error(
GraphQLError(
f"Query depth exceeds the maximum allowed depth of {self.max_depth}.",
node,
)
)

def leave_field(self, node: FieldNode, *_args: Any) -> None:
self.operation_depth -= 1

return MaxDepthRule


def create_max_aliases_rule(max_aliases: int) -> Type[ValidationRule]:
class MaxAliasesRule(ValidationRule):
def __init__(self, context: ValidationContext) -> None:
super().__init__(context)
self.alias_count: int = 0
self.has_reported_error: bool = False
self.max_aliases: int = max_aliases

def enter_document(self, node: DocumentNode, *_args: Any) -> None:
self.alias_count = 0
self.has_reported_error = False

def enter_field(self, node: FieldNode, *_args: Any) -> None:
if node.alias:
self.alias_count += 1

if self.alias_count > self.max_aliases and not self.has_reported_error:
self.has_reported_error = True
self.report_error(
GraphQLError(
f"Query uses too many aliases. Maximum allowed is {self.max_aliases}.",
node,
)
)

return MaxAliasesRule
7 changes: 5 additions & 2 deletions graphql_api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from services.redis_configuration import get_redis_connection

from .schema import schema
from .validation import create_max_aliases_rule, create_max_depth_rule

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -188,7 +189,7 @@ def __exit__(self, exc_type, exc_value, exc_traceback):
class AsyncGraphqlView(GraphQLAsyncView):
schema = schema
extensions = [QueryMetricsExtension]
introspection = getattr(settings, "GRAPHQL_INTROSPECTION_ENABLED", False)
introspection = settings.GRAPHQL_INTROSPECTION_ENABLED

def get_validation_rules(
self,
Expand All @@ -201,7 +202,9 @@ def get_validation_rules(
maximum_cost=settings.GRAPHQL_QUERY_COST_THRESHOLD,
default_cost=1,
variables=data.get("variables"),
)
),
create_max_depth_rule(max_depth=settings.GRAPHQL_MAX_DEPTH),
create_max_aliases_rule(max_aliases=settings.GRAPHQL_MAX_ALIASES),
]

validation_rules = get_validation_rules # type: ignore
Expand Down
Loading