OrderingFilter triggers a 500 error if a single comma is given as the input

[munnsmunns]

When ',' is given as the value for an OrderingFilter in a filterset form, a FieldError is thrown: Cannot resolve keyword '' into field. Choices are: . . . It appears that bots are hitting a search page on our site with ?order_by=%2c in the url which triggers this behavior

I would expect the input to be rejected without throwing an uncaught exception, which is the behavior when an invalid field name is given as the input for the OrderingFilter. It appears it's being interpreted as a CSV input instead.

Our use of the OrderingFilter class:

order_by = df.OrderingFilter(
    empty_label="Default Order",
    fields=(
        ("authors", "authors"),
        ("year", "year"),
    ),
    help_text="How results will be ordered",
)

[carltongibson]

So the widget should be returning an empty list (rather than a list of what, empty strings? 🤔)

Can you put together a test case demonstrating this issue and we can see what the tweak might be?

Thanks.

[munnsmunns]

class F(FilterSet):
    order_by = OrderingFilter(
        fields=("title",),
    )

    class Meta:
        model = Book
        fields = ("title",)

qs = Book.objects.all()
f = F(data={"order_by": ","}, queryset=qs)

f.qs # this will raise a FieldError when it tries to do qs.order_by("")

# instead I would instead expect a validation error was raised in f.form.errors["order_by"] or that
# f.form.cleaned_data["order_by"] would be an empty list

It does seem the widget is returning a list of empty strings so returning an empty list would fix it. It might also make sense to change the clean method to raise a ValidationError if there are empty strings in the list. This would handle other cases, like if "title," was given as the input

[carltongibson]

Ok, fancy putting that as a test case in a pull request?

[carltongibson]

Hi @munnsmunns — I've been playing with this.

Can I ask that you show me a set up that triggers the error here?

Adjusting the test case from your PR, like so…

diff --git a/django_filters/filters.py b/django_filters/filters.py
index bbf950c..16ada96 100644
--- a/django_filters/filters.py
+++ b/django_filters/filters.py
@@ -719,7 +719,7 @@ class OrderingFilter(BaseCSVFilter, ChoiceFilter):

     """

-    base_field_class = BaseOrderingField
+#     base_field_class = BaseOrderingField
     descending_fmt = _("%s (descending)")

     def __init__(self, *args, **kwargs):
diff --git a/tests/test_filtering.py b/tests/test_filtering.py
index b048b56..11e4854 100644
--- a/tests/test_filtering.py
+++ b/tests/test_filtering.py
@@ -1979,17 +1979,26 @@ class OrderingFilterTests(TestCase):

     def test_csv_input(self):
         class F(FilterSet):
-            o = OrderingFilter(widget=forms.Select, fields=("username",))
+            o = OrderingFilter(widget=forms.Select, fields=("username",),)

             class Meta:
                 model = User
                 fields = ["username"]

         qs = User.objects.all()
-        f = F({"o": ","}, queryset=qs)
-        f.qs
-        value = f.form.cleaned_data["o"]
-        self.assertEqual(value, [])
+        tests = [
+            {"o": ","},
+            QueryDict("order_by=%2c"),
+            QueryDict("order_by=,"),
+        ]
+        for data in tests:
+            with self.subTest(data=data):
+                f = F(data, queryset=qs)
+#                 self.assertIs(None, f.form["o"].field.widget.value_from_datadict(f.data, {}, 'o'))
+                self.assertIs(True, f.is_valid())
+                names = f.qs.values_list("username", flat=True)
+                self.assertEqual(list(names), ['alex', 'jacob', 'aaron', 'carl'])
+

 class MiscFilterSetTests(TestCase):
     def setUp(self):

Both the QueryDict examples pass, and it's only the dict example {"o": ","} that triggers the error.

You wrote:

It appears that bots are hitting a search page on our site with ?order_by=%2c in the url which triggers this behavior

But in this case I'm expecting to the QueryDict("order_by=%2c") behaviour, since that's what your filterset should be instantiated with (request.GET)

Thanks!

[scott-8]

@carltongibson The tests with QueryDict work as expected (trigger the error) when you rename the filter to order_by to match the filter name.

Here's the test I ran:

def test_csv_input(self):
    class F(FilterSet):
        order_by = OrderingFilter(widget=forms.Select, fields=("username",),)

        class Meta:
            model = User
            fields = ["username"]

    qs = User.objects.all()
    tests = [
        QueryDict("order_by=%2c"),
        QueryDict("order_by=,"),
    ]
    for data in tests:
        with self.subTest(data=data):
            f = F(data, queryset=qs)
            # self.assertIs(None, f.form["o"].field.widget.value_from_datadict(f.data, {}, 'o'))
            self.assertIs(True, f.is_valid())
            names = f.qs.values_list("username", flat=True)
            self.assertEqual(list(names), ['alex', 'jacob', 'aaron', 'carl'])

[munnsmunns]

@carltongibson Sorry I haven't responded in a while, @scott-8 's post is perfect for triggering the error I was talking about

[carltongibson]

Great. Thanks. I'll take another look.

[scott-8]

One other test case that might be useful to add would be a trailing comma to a filter. Adding this query to the tests list above also triggers the error: QueryDict("order_by=username,").