500 error for modeladmin pages with unexpected get attrs

[AdrienLemaire]

Found a bug? Please fill out the sections below. 👍

Issue Summary

When accessible a url from a third-part service, unwanted get keywords may be appended to the url, eg: /?utm_campaign=website&utm_source=sendgrid.com&utm_medium=email

The currently logic of modeladmin.views.IndexView is to filtered out IGNORED_PARAMS (order, order_type, search vars) then send all remaining filters to the queryset.

wagtail/contrib/modeladmin/views.py

class IndexView(WMABaseView):
    IGNORED_PARAMS = (ORDER_VAR, ORDER_TYPE_VAR, SEARCH_VAR)

    def get_filters_params(self, params=None):
        for ignored in self.IGNORED_PARAMS:
            if ignored in lookup_params:
                del lookup_params[ignored]
        return lookup_params

    def get_filters(self, request):
        lookup_params = self.get_filters_params()

    def get_queryset(self, request=None):
        # First, we collect all the declared list filters.
        (self.filter_specs, self.has_filters, remaining_lookup_params,
         filters_use_distinct) = self.get_filters(request)
    try:
            # Finally, we apply the remaining lookup parameters from the query
            # string (i.e. those that haven't already been processed by the
            # filters).
            qs = qs.filter(**remaining_lookup_params)
        except (SuspiciousOperation, ImproperlyConfigured):
            # Allow certain types of errors to be re-raised as-is so that the
            # caller can treat them in a special way.
            raise
        except Exception as e:
            # Every other error is caught with a naked except, because we don't
            # have any other way of validating lookup parameters. They might be
            # invalid if the keyword arguments are incorrect, or if the values
            # are not in the correct type, so we might get FieldError,
            # ValueError, ValidationError, or ?.
            raise IncorrectLookupParameters(e)

Steps to Reproduce

1. Create a simple custom ModelAdmin (class MyModelAdmin(ModelAdmin):)
2. Access the indexview for that model.
3. Append some random get param /?a=1 and refresh.

django.contrib.admin.options.IncorrectLookupParameters
django.contrib.admin.options.IncorrectLookupParameters: Cannot resolve keyword 'a' into field. Choices are: …

Traceback (most recent call last)

Any other relevant information. For example, why do you consider this a bug and what did you expect to happen instead?

I would have expected a whitelist of authorized filters instead, or a way to ignore incorrect params. The system shouldn't break when a user add unexpected get params to the url.

• I have confirmed that this issue can be reproduced as described on a fresh Wagtail project: (no)

Technical details

• Python version: Python 3.7.4
• Django version: Django==2.2.7
• Wagtail version: wagtail==2.7
• Browser version: Chrome 78.0

[AdrienLemaire]

Quick and dirty temporary fix in my project until this issue is resolved:

class StrictIndexView(IndexView):
    def get_filters(self, request: HttpRequest) -> tuple:
        """
        Override get_filters to remove sendgrid filters
        """
        (
            self.filter_specs,
            self.has_filters,
            remaining_lookup_params,
            filters_use_distinct,
        ) = super().get_filters(request)
        remaining_lookup_params = {
            k: v
            for k, v in remaining_lookup_params.items()
            if not k.startswith("utm")
        }
        return (
            self.filter_specs,
            self.has_filters,
            remaining_lookup_params,
            filters_use_distinct,
        )

class MyModelAdmin(ModelAdmin):
    model = MyModel
    index_view_class = StrictIndexView

[ababic]

ModelAdmin is only intended to be used within the Wagtail admin, and so I wouldn't class this as a bug necessarily. Users shouldn't be adding random query params to URLs either - for every view in wagtail to safely ignore all user-added GET parameters would be a reasonable chunk of work, and something I don't think we'd want to make promises about going forward.

That said, I would be happy to review a pull request that implemented a change along these lines, so long as there wasn't a significant affect on performance. @AdrienLemaire would you be up for creating a PR?