From 71c03b9db97edbde228777981de0ac7b664302de Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Wed, 15 Jan 2014 14:27:41 +0000 Subject: [PATCH] Security update to OrderingFilter --- docs/api-guide/filtering.md | 26 ++++++- docs/topics/release-notes.md | 7 ++ rest_framework/__init__.py | 2 +- rest_framework/filters.py | 29 ++++++-- rest_framework/tests/test_filters.py | 104 ++++++++++++++++++++++++++- 5 files changed, 160 insertions(+), 8 deletions(-) diff --git a/docs/api-guide/filtering.md b/docs/api-guide/filtering.md index 0e02a2a7d..07420d842 100644 --- a/docs/api-guide/filtering.md +++ b/docs/api-guide/filtering.md @@ -282,13 +282,37 @@ Multiple orderings may also be specified: http://example.com/api/users?ordering=account,username +### Specifying which fields may be ordered against + +It's recommended that you explicitly specify which fields the API should allowing in the ordering filter. You can do this by setting an `ordering_fields` attribute on the view, like so: + + class UserListView(generics.ListAPIView): + queryset = User.objects.all() + serializer_class = UserSerializer + filter_backends = (filters.OrderingFilter,) + ordering_fields = ('username', 'email') + +This helps prevent unexpected data leakage, such as allowing users to order against a password hash field or other sensitive data. + +If you *don't* specify an `ordering_fields` attribute on the view, the filter class will default to allowing the user to filter on any readable fields on the serializer specified by the `serializer_class` attribute. + +If you are confident that the queryset being used by the view doesn't contain any sensitive data, you can also explicitly specify that a view should allow ordering on *any* model field or queryset aggregate, by using the special value `'__all__'`. + + class BookingsListView(generics.ListAPIView): + queryset = Booking.objects.all() + serializer_class = BookingSerializer + filter_backends = (filters.OrderingFilter,) + ordering_fields = '__all__' + +### Specifying a default ordering + If an `ordering` attribute is set on the view, this will be used as the default ordering. Typically you'd instead control this by setting `order_by` on the initial queryset, but using the `ordering` parameter on the view allows you to specify the ordering in a way that it can then be passed automatically as context to a rendered template. This makes it possible to automatically render column headers differently if they are being used to order the results. class UserListView(generics.ListAPIView): queryset = User.objects.all() - serializer = UserSerializer + serializer_class = UserSerializer filter_backends = (filters.OrderingFilter,) ordering = ('username',) diff --git a/docs/topics/release-notes.md b/docs/topics/release-notes.md index cd87c7b2d..14503148c 100644 --- a/docs/topics/release-notes.md +++ b/docs/topics/release-notes.md @@ -40,6 +40,13 @@ You can determine your currently installed version using `pip freeze`: ## 2.3.x series +### 2.3.12 + +**Date**: 15th January 2014 + +* **Security fix**: `OrderingField` now only allows ordering on readable serializer fields, or on fields explicitly specified using `ordering_fields`. This prevents users being able to order by fields that are not visible in the API, and exploiting the ordering of sensitive data such as password hashes. +* Bugfix: `write_only = True` fields now display in the browsable API. + ### 2.3.11 **Date**: 14th January 2014 diff --git a/rest_framework/__init__.py b/rest_framework/__init__.py index 883f24a2e..6759680b7 100644 --- a/rest_framework/__init__.py +++ b/rest_framework/__init__.py @@ -8,7 +8,7 @@ ______ _____ _____ _____ __ _ """ __title__ = 'Django REST framework' -__version__ = '2.3.11' +__version__ = '2.3.12' __author__ = 'Tom Christie' __license__ = 'BSD 2-Clause' __copyright__ = 'Copyright 2011-2013 Tom Christie' diff --git a/rest_framework/filters.py b/rest_framework/filters.py index 5c6a187c4..de91caedc 100644 --- a/rest_framework/filters.py +++ b/rest_framework/filters.py @@ -3,6 +3,7 @@ Provides generic filtering backends that can be used to filter the results returned by list views. """ from __future__ import unicode_literals +from django.core.exceptions import ImproperlyConfigured from django.db import models from rest_framework.compat import django_filters, six, guardian, get_model_name from functools import reduce @@ -107,6 +108,7 @@ class SearchFilter(BaseFilterBackend): class OrderingFilter(BaseFilterBackend): ordering_param = 'ordering' # The URL query parameter used for the ordering. + ordering_fields = None def get_ordering(self, request): """ @@ -122,17 +124,34 @@ class OrderingFilter(BaseFilterBackend): return (ordering,) return ordering - def remove_invalid_fields(self, queryset, ordering): - field_names = [field.name for field in queryset.model._meta.fields] - field_names += queryset.query.aggregates.keys() - return [term for term in ordering if term.lstrip('-') in field_names] + def remove_invalid_fields(self, queryset, ordering, view): + valid_fields = getattr(view, 'ordering_fields', self.ordering_fields) + + if valid_fields is None: + # Default to allowing filtering on serializer fields + serializer_class = getattr(view, 'serializer_class') + if serializer_class is None: + msg = ("Cannot use %s on a view which does not have either a " + "'serializer_class' or 'ordering_fields' attribute.") + raise ImproperlyConfigured(msg % self.__class__.__name__) + valid_fields = [ + field.source or field_name + for field_name, field in serializer_class().fields.items() + if not getattr(field, 'write_only', False) + ] + elif valid_fields == '__all__': + # View explictly allows filtering on any model field + valid_fields = [field.name for field in queryset.model._meta.fields] + valid_fields += queryset.query.aggregates.keys() + + return [term for term in ordering if term.lstrip('-') in valid_fields] def filter_queryset(self, request, queryset, view): ordering = self.get_ordering(request) if ordering: # Skip any incorrect parameters - ordering = self.remove_invalid_fields(queryset, ordering) + ordering = self.remove_invalid_fields(queryset, ordering, view) if not ordering: # Use 'ordering' attribute by default diff --git a/rest_framework/tests/test_filters.py b/rest_framework/tests/test_filters.py index 614f45cc7..181881865 100644 --- a/rest_framework/tests/test_filters.py +++ b/rest_framework/tests/test_filters.py @@ -368,7 +368,6 @@ class OrderingFilterRelatedModel(models.Model): related_name="relateds") - class OrderingFilterTests(TestCase): def setUp(self): # Sequence of title/text is: @@ -394,6 +393,7 @@ class OrderingFilterTests(TestCase): model = OrdringFilterModel filter_backends = (filters.OrderingFilter,) ordering = ('title',) + ordering_fields = ('text',) view = OrderingListView.as_view() request = factory.get('?ordering=text') @@ -412,6 +412,7 @@ class OrderingFilterTests(TestCase): model = OrdringFilterModel filter_backends = (filters.OrderingFilter,) ordering = ('title',) + ordering_fields = ('text',) view = OrderingListView.as_view() request = factory.get('?ordering=-text') @@ -430,6 +431,7 @@ class OrderingFilterTests(TestCase): model = OrdringFilterModel filter_backends = (filters.OrderingFilter,) ordering = ('title',) + ordering_fields = ('text',) view = OrderingListView.as_view() request = factory.get('?ordering=foobar') @@ -448,6 +450,7 @@ class OrderingFilterTests(TestCase): model = OrdringFilterModel filter_backends = (filters.OrderingFilter,) ordering = ('title',) + oredering_fields = ('text',) view = OrderingListView.as_view() request = factory.get('') @@ -466,6 +469,7 @@ class OrderingFilterTests(TestCase): model = OrdringFilterModel filter_backends = (filters.OrderingFilter,) ordering = 'title' + ordering_fields = ('text',) view = OrderingListView.as_view() request = factory.get('') @@ -494,6 +498,7 @@ class OrderingFilterTests(TestCase): model = OrdringFilterModel filter_backends = (filters.OrderingFilter,) ordering = 'title' + ordering_fields = '__all__' queryset = OrdringFilterModel.objects.all().annotate( models.Count("relateds")) @@ -510,4 +515,101 @@ class OrderingFilterTests(TestCase): ) +class SensitiveOrderingFilterModel(models.Model): + username = models.CharField(max_length=20) + password = models.CharField(max_length=100) + +# Three different styles of serializer. +# All should allow ordering by username, but not by password. +class SensitiveDataSerializer1(serializers.ModelSerializer): + username = serializers.CharField() + + class Meta: + model = SensitiveOrderingFilterModel + fields = ('id', 'username') + + +class SensitiveDataSerializer2(serializers.ModelSerializer): + username = serializers.CharField() + password = serializers.CharField(write_only=True) + + class Meta: + model = SensitiveOrderingFilterModel + fields = ('id', 'username', 'password') + + +class SensitiveDataSerializer3(serializers.ModelSerializer): + user = serializers.CharField(source='username') + + class Meta: + model = SensitiveOrderingFilterModel + fields = ('id', 'user') + + +class SensitiveOrderingFilterTests(TestCase): + def setUp(self): + for idx in range(3): + username = {0: 'userA', 1: 'userB', 2: 'userC'}[idx] + password = {0: 'passA', 1: 'passC', 2: 'passB'}[idx] + SensitiveOrderingFilterModel(username=username, password=password).save() + + def test_order_by_serializer_fields(self): + for serializer_cls in [ + SensitiveDataSerializer1, + SensitiveDataSerializer2, + SensitiveDataSerializer3 + ]: + class OrderingListView(generics.ListAPIView): + queryset = SensitiveOrderingFilterModel.objects.all().order_by('username') + filter_backends = (filters.OrderingFilter,) + serializer_class = serializer_cls + + view = OrderingListView.as_view() + request = factory.get('?ordering=-username') + response = view(request) + + if serializer_cls == SensitiveDataSerializer3: + username_field = 'user' + else: + username_field = 'username' + + # Note: Inverse username ordering correctly applied. + self.assertEqual( + response.data, + [ + {'id': 3, username_field: 'userC'}, + {'id': 2, username_field: 'userB'}, + {'id': 1, username_field: 'userA'}, + ] + ) + + def test_cannot_order_by_non_serializer_fields(self): + for serializer_cls in [ + SensitiveDataSerializer1, + SensitiveDataSerializer2, + SensitiveDataSerializer3 + ]: + class OrderingListView(generics.ListAPIView): + queryset = SensitiveOrderingFilterModel.objects.all().order_by('username') + filter_backends = (filters.OrderingFilter,) + serializer_class = serializer_cls + + view = OrderingListView.as_view() + request = factory.get('?ordering=password') + response = view(request) + + if serializer_cls == SensitiveDataSerializer3: + username_field = 'user' + else: + username_field = 'username' + + # Note: The passwords are not in order. Default ordering is used. + self.assertEqual( + response.data, + [ + {'id': 1, username_field: 'userA'}, # PassB + {'id': 2, username_field: 'userB'}, # PassC + {'id': 3, username_field: 'userC'}, # PassA + ] + ) \ No newline at end of file