Security update to OrderingFilter2.3.12

5 files changed, 160 insertions, 8 deletions

diff --git a/docs/api-guide/filtering.md b/docs/api-guide/filtering.md
index 0e02a2a7..07420d84 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 cd87c7b2..14503148 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 883f24a2..6759680b 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 5c6a187c..de91caed 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 614f45cc..18188186 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