Django-rest-framework: ValueError on incorrect user input, should we sanitize user input by default?

[niksite]

Checklist

• [X] I have verified that that issue exists against the master branch of Django REST framework.
• [X] I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• [X] This is not a usage question. (Those should be directed to the discussion group instead.)
• [X] This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
• [X] I have reduced the issue to the simplest possible case.
• [ ] I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

Any viewsets.ReadOnlyModelViewSet with search_fields attribute.
Request the API endpoint with the following search query param:
api-url?search=Exploit%20String:%200%0d%0a%00%3Cscript%20src=//example.com%3E

Expected behavior

No match found, no exception raised.
Expecting to see something like {"count": 0, next: None, previous: None, results: []} in the output.

Actual behavior

Got HTTP 500 error with the following exception:
ValueError: A string literal cannot contain NUL (0x00) characters.

[xordoquy]

Do you have the full traceback ?

[niksite]

Sure. In my current case this is the following (not sure how to make it more 'full'):

Traceback:  
File "/app/env/lib/python2.7/site-packages/django/core/handlers/exception.py" in inner
 41.             response = get_response(request)
File "/app/env/lib/python2.7/site-packages/django/core/handlers/base.py" in _get_response
 187.                 response = self.process_exception_by_middleware(e, request)
File "/app/env/lib/python2.7/site-packages/django/core/handlers/base.py" in _get_response
 185.                 response = wrapped_callback(request, *callback_args, **callback_kwargs)
File "/app/env/lib/python2.7/site-packages/django/views/decorators/csrf.py" in wrapped_view
 58.         return view_func(*args, **kwargs)
File "/app/env/lib/python2.7/site-packages/rest_framework/viewsets.py" in view
 116.             return self.dispatch(request, *args, **kwargs)
File "/app/env/lib/python2.7/site-packages/rest_framework/views.py" in dispatch
 495.             response = self.handle_exception(exc)
File "/app/env/lib/python2.7/site-packages/rest_framework/views.py" in handle_exception
 455.             self.raise_uncaught_exception(exc)
File "/app/env/lib/python2.7/site-packages/rest_framework/views.py" in dispatch
 492.             response = handler(request, *args, **kwargs)
File "/app/env/lib/python2.7/site-packages/rest_framework/mixins.py" in list
 42.         page = self.paginate_queryset(queryset)
File "/app/env/lib/python2.7/site-packages/rest_framework/generics.py" in paginate_queryset
 173.         return self.paginator.paginate_queryset(queryset, self.request, view=self)
File "/app/env/lib/python2.7/site-packages/rest_framework/pagination.py" in paginate_queryset
 325.         self.count = self.get_count(queryset)
File "/app/env/lib/python2.7/site-packages/rest_framework/pagination.py" in get_count
 466.             return queryset.count()
File "/app/env/lib/python2.7/site-packages/cacheops/query.py" in count
 307.             return self._no_monkey.count(self)
File "/app/env/lib/python2.7/site-packages/django/db/models/query.py" in count
 364.         return self.query.get_count(using=self.db)
File "/app/env/lib/python2.7/site-packages/django/db/models/sql/query.py" in get_count
 499.         number = obj.get_aggregation(using, ['__count'])['__count']
File "/app/env/lib/python2.7/site-packages/django/db/models/sql/query.py" in get_aggregation
 480.         result = compiler.execute_sql(SINGLE)
File "/app/env/lib/python2.7/site-packages/django/db/models/sql/compiler.py" in execute_sql
 899.             raise original_exception

Exception Type: ValueError at /api/apps/
Exception Value: A string literal cannot contain NUL (0x00) characters.

[carltongibson]

There's a related bug on Django about this. https://code.djangoproject.com/ticket/30064.

The summary there is that this should be handled by a form (etc) sanitising the user input before passing it to the ORM. (That seems reasonable to me.)

[tomchristie]

Would be totally reasonable for our string fields to strip null characters, yup.

[rpkilby]

Is this not fixed by #6073?

[tomchristie]

Looks like it, yes.

@niksite - Could you verify which version of REST framework you were running here?

Can we verify this against master now or not?

[niksite]

1. I am using 3.9.1 version of DRF.
2. This ticket is about '?search=' parameter to the query. Is it working via regular CharField too?

[carltongibson]

6073 just added the validator to serializer fields. But SearchFilter doesn't use a serializer or form to sanitize the query parameter, so bad values can leak in.

https://github.com/encode/django-rest-framework/blob/d110454d4c15fe6617a112e846b89e09ed6c95b2/rest_framework/filters.py#L64-L70

This is the same thing that happens in the Django admin in the linked ticket.

We basically should be using a form on the parameters before using then. ("Sanitize input...")

[busla]

Here is a quick draft of how this could be done by using form sanitation. Please be kind :smile: I might be totally out of order here.

https://github.com/encode/django-rest-framework/compare/master...busla:sanitize-searchfield-input?expand=1

[busla]

Wondering if I should raise a custom exception when is_valid is False or try to propogate the Validation error?

After looking through the code flow of filters.py, it might be acceptable to ignore form.errors and not raise a ValidationError, instead just return an empty list as before.