django-rest-framework-filters icon indicating copy to clipboard operation
django-rest-framework-filters copied to clipboard

Using default queryset is insecure when filtering across relationships

Open rpkilby opened this issue 9 years ago • 4 comments

Filtering across relationships currently uses the default queryset. For example:

class ArticleFilter(FilterSet):
    is_published = filers.BooleanFilter()
    body = filter.CharFilter()

class AuthorFilter(FilterSet):
    article = filters.RelatedFilter(ArticleFilter)

class AuthorViewSet(viewsets.ModelViewSet):
    queryset = models.Author.objects.all()
    filter_class = AuthorFilter

class PostViewSet(viewsets.ModelViewSet):
    queryset = models.Post.objects.all()
    filter_class = PostFilter

    def get_queryset(self):
        qs = super(PostViewSet, self).get_queryset()
        user = self.request.user

        # get user's posts and published posts
       user_posts = qs.filter(author__user=user)
       published_posts = qs.filter(is_published=True)

       return user_posts | published_posts

While you're not able to view unpublished posts of other authors, you are able to filter across the relationship and derive that an author may be preparing an article about some topic.

eg:

/api/authors?post__is_published=false&post__body__contains=juicy%20story%20details

rpkilby avatar Jul 31 '16 18:07 rpkilby

This also leaks information in the rendered form, as the dropdown for the ModelChoiceFilter will contain the entire queryset. eg, a related product filter might display your entire customer list.

rpkilby avatar Aug 12 '16 17:08 rpkilby

It should be documented that a RelatedFilter.queryset should be consistent with the logic applied in ViewSet.get_queryset(). As explained above, an inconsitency would have the potential to leak data.

My recommendation would be to write queryset methods that provides the set of objects that a user should have read access to, then use that queryset method where necessary. e.g.,

# Get authors/books for which users have read permissions
class AuthorQuerySet(models.QuerySet):
    def for_user(self, user):
        return self.filter(something=user)

class BookQuerySet(models.QuerySet):
    def for_user(self, user):
        return self.filter(something=user)

# filters
class AuthorFilterSet:
    ...

class BookFilterSet:
    author = RelatedFilter(queryset=Author.objects.for_request)
    ...

# views
class AuthorViewSet:
    filterset_class = AuthorFilterSet
    def get_queryset(self):
        return Author.objects.for_request(self.request)

class BookViewSet:
    filterset_class = BookFilterSet
    def get_queryset(self):
        return Book.objects.for_request(self.request)

rpkilby avatar Jul 14 '18 01:07 rpkilby

@rpkilby when using related filter I can pass a queryset to it and it works. Like this for example:

    house__furniture__electrical = RelatedFilter(SomeFilter, queryset=some_custom_queryset)

So I think this issue is resolved and can be closed, what do you think?

sassanh avatar Oct 25 '19 08:10 sassanh

Hi @sassanh. Thanks - this is more a reminder that the docs need to be updated to better inform the user on how to use RelatedFilter.queryset.

rpkilby avatar Oct 25 '19 17:10 rpkilby