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

Add query cost analysis and limiting #101

Closed
iamareebjamal opened this issue Dec 4, 2019 · 5 comments
Closed

Add query cost analysis and limiting #101

iamareebjamal opened this issue Dec 4, 2019 · 5 comments

Comments

@iamareebjamal
Copy link

As the data fetched is controlled by client, and not the server, a malicious client can create a very expensive query with lots of nested fields and joins and can bring down the server. There must be a way to limit the amount of resources to be fetched

https://developer.github.com/v4/guides/resource-limitations/

@ashleyredzko
Copy link
Collaborator

This technically sounds like a DRF functionality. Is the goal to reduce the number returned to the view? That's usually handled via pagination. Otherwise, is this for many=True nested serializers?

@iamareebjamal
Copy link
Author

iamareebjamal commented Dec 5, 2019

No, the goal is to prevent denial of service attacks on the server by preventing malicious queries by client.

https://blog.apollographql.com/securing-your-graphql-api-from-malicious-queries-16130a324a6b
https://sangria-graphql.org/learn/#limiting-query-depth
graphql-python/graphene#907
mirumee/ariadne#40

@iamareebjamal iamareebjamal changed the title Add resource rate limiting Add query cost analysis and limiting Dec 5, 2019
@ashleyredzko
Copy link
Collaborator

I'm unsure that applies to this library. django-restql basically allows you to manipulate the fields returned from your DRF serializers via a query parameter (which can help narrow down what you need). It doesn't allow you to ask for fields that aren't already present and does not manipulate the number of results returned (since your view and serializer control that). As such, there's no way to ask for a field that would be a denial of service that also wouldn't be a denial of service on a traditional serializer without django-restql support.

Granted, when using graphql, you definitely need these safeguards because it's an open book basically.

In terms of trying to prevent a case like this, there's the EagerLoadingMixin from django-restql and traditional ORM and Serializer optimizations available via Django and Django Rest Framework.

@ashleyredzko
Copy link
Collaborator

ashleyredzko commented Dec 5, 2019

To futher expand on this, imagine you have two serializers, PersonSerializer and CarSerializer. Please forgive any typos or semantics I glossed over. Typing this on the fly and just trying to illustrate an idea.

class CarSerializer(DynamicFieldsSerializer, serializers.ModelSerialier):
    owner = PersonSerializer(read_only=True)

    class Meta:
        model = Car
        fields = ("id", "make", "model", "owner")

class Person(DynamicFieldsSerializer, serializers.ModelSerializer):
    cars = CarSerializer(many=True, read_only=True)

    class Meta:
        model = Person
        fields = ("id", "first_name", "last_name", "cars")

This is an infinite loop. Person renders a list of full car serializers, which renders the full person serializer, which renders the full car serializer... ad nauseum.

One way to fix this with django-restql is to exclude fields that would force the recursion to happen.

class CarSerializer(DynamicFieldsSerializer, serializers.ModelSerialier):
    owner = PersonSerializer(exclude=["cars"], read_only=True)

    class Meta:
        model = Car
        fields = ("id", "make", "model", "owner")

class Person(DynamicFieldsSerializer, serializers.ModelSerializer):
    cars = CarSerializer(exclude=["owner"], many=True, read_only=True)

    class Meta:
        model = Person
        fields = ("id", "first_name", "last_name", "cars")

This way, we don't render the related serializers in full, avoiding the infinite loop. This also prevents querying for that field since it's excluded. As such, a query like ?query={person{cars{owner...}}} would error at owner, since it's an excluded field at the person level.

Note that this example wouldn't be possible as-is without django-restql. You'd normally have to write your own serializer or method field to avoid this. Many DRF apis actually prefer the hyperlinked/primary key approach to related objects to avoid this issue as well.

@iamareebjamal
Copy link
Author

Hmm, got it. Thanks.

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

No branches or pull requests

2 participants