site icon indicating copy to clipboard operation
site copied to clipboard

Reject API GET Requests with invalid fields

Open sco1 opened this issue 6 years ago • 2 comments

As tangentially referenced in https://github.com/python-discord/bot/issues/445, the API will silently ignore any invalid fields when filtering the values to return. I think it might be a better idea to reject GET requests with invalid fields in order to avoid, say, dumping the entire infraction list into the response if one of the more broad filters is incorrectly specified.

sco1 avatar Sep 24 '19 17:09 sco1

~~I've looked, but I haven't found a built-in way to do this with DRF. Since a lot of the serializers already have some sort of custom validation function, I think the easiest way forward is to add a decorator that compares the keys of the received data with the fields specified in the model before it runs the regular validate method of the serialized.~~

I'll have a look to see if I can come up with something that doesn't look too bad.

SebastiaanZ avatar Sep 30 '19 20:09 SebastiaanZ

The more I look into this, the more complicated it becomes. The main problem is that there isn't a single unit that processes the GET parameters, but multiple independent "filter backends" that are all unaware of the parameters used by the other "filter backends" in use or even which filters are used for that view at all.

As an example, the infraction viewset uses these filter back-end settings¹:

    filter_backends = (DjangoFilterBackend, SearchFilter, OrderingFilter)
    filterset_fields = ('user__id', 'actor__id', 'active', 'hidden', 'type')
    search_fields = ('$reason',)

All the filters will modify the actual query that's going to be run separately, meaning that the DjangoFilterBackend will look for the specified fields in the params and add a filter() to the query based on the values, the SearchFitler will add something similar, but with a regex seach for reason if that param is present, and the OrderingFilter will look for a ordering key in the params and, if present, will add a order_by to the query that's being generated.

None of them are aware of the others, so none of them individually can say "Hey, there are too many param keys present, this request should be rejected". This is why I think the default behavior is as it is, it's difficult to determine if anything is looking for a certain param key or not.

A way around this would be for us to manually compare the GET param keys against a whitelist, but that whitelist is difficult to generate automatically. While the settings above include the DjangoFilterBackend as-is, we'd need to parse the search_fields values to extract the field name from the potential "search type specifier" (currently things like $, = to indicate regex matching, exact matching, and so on), and add the params OrderingField is looking for (ordering by default in the current version, but this could be overwritten in settings.py) manually.

We could add an explicit whitelist per ListView, but that means we'd have to be very careful to specify the params we want to allow and adapt it whenever we remove or introduce a filter back-end. I do think this is the only real way forward if we do want to have whitelists for params (which is another way of looking at "rejecting all requests with unknown fields").


  1. ~~It actually has filter_fields not filterset_fields, but after diving into the documentation for the filter back-end, I think that's incorrect and doesn't really do anything.~~ Nevermind, they both work as expected, but using filter_fields like this is not mentioned in the documentation.

SebastiaanZ avatar Oct 09 '19 07:10 SebastiaanZ

We discussed this in the site meeting today and decided that due to the reasons Sebastiaan mentioned it's infeasible to implement this unfortunately.

jchristgit avatar Mar 21 '23 20:03 jchristgit