AltSchool / dynamic-rest

Dynamic extensions for Django REST Framework
MIT License
820 stars 107 forks source link

Crash in `WithDynamicViewSetMixin.get_request_fields` when matching `include[]` substrings #364

Open jamesbraza opened 6 months ago

jamesbraza commented 6 months ago

With Django Dynamic REST version https://github.com/AltSchool/dynamic-rest/tree/v2.1.7, there is the possibility for a crash based on two include[] fields matching the same substring. This can be reproduced in the test suite's test_api:

    def test_include_matching_substring(self) -> None:
        self.client.get(
            '/users/'
            '?include[]=location.cats'
            '&include[]=location.cats.home'
        )

This will lead to a crash per TypeError("'bool' object does not support item assignment"):

Traceback (most recent call last):
  File "/Users/user/code/dynamic-rest/tests/test_api.py", line 810, in test_foo
    self.client.get(url)
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/rest_framework/test.py", line 289, in get
    response = super().get(path, data=data, **extra)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/rest_framework/test.py", line 206, in get
    return self.generic('GET', path, **r)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/rest_framework/test.py", line 234, in generic
    return super().generic(
           ^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/django/test/client.py", line 541, in generic
    return self.request(**r)
           ^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/rest_framework/test.py", line 286, in request
    return super().request(**kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/rest_framework/test.py", line 238, in request
    request = super().request(**kwargs)
              ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/django/test/client.py", line 810, in request
    self.check_exception(response)
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/django/test/client.py", line 663, in check_exception
    raise exc_value
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/django/core/handlers/exception.py", line 56, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/django/views/decorators/csrf.py", line 55, in wrapped_view
    return view_func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/rest_framework/viewsets.py", line 125, in view
    return self.dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/rest_framework/views.py", line 509, in dispatch
    response = self.handle_exception(exc)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/rest_framework/views.py", line 469, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
    raise exc
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/rest_framework/views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/tests/viewsets.py", line 54, in list
    return super(UserViewSet, self).list(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/rest_framework/mixins.py", line 38, in list
    queryset = self.filter_queryset(self.get_queryset())
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/venv/lib/python3.11/site-packages/rest_framework/generics.py", line 150, in filter_queryset
    queryset = backend().filter_queryset(self.request, queryset, self)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/dynamic_rest/filters.py", line 236, in filter_queryset
    return self._build_queryset(
           ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/dynamic_rest/filters.py", line 522, in _build_queryset
    serializer = self.view.get_serializer()
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/dynamic_rest/viewsets.py", line 317, in get_serializer
    kwargs['request_fields'] = self.get_request_fields()
                               ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/code/dynamic-rest/dynamic_rest/viewsets.py", line 256, in get_request_fields
    current_fields[segment] = include
    ~~~~~~~~~~~~~~^^^^^^^^^
TypeError: 'bool' object does not support item assignment

I think this should either:


A short term workaround is using a Django middleware:

# middleware.py

import traceback
from collections.abc import Callable

from django.http import HttpRequest, HttpResponse, HttpResponseBadRequest

class DuplicateIncludeExcludeFieldSubstringsMiddleware:
    """
    Safely handle duplicate include[] or exclude[] field substrings in query params.

    SEE: https://github.com/AltSchool/dynamic-rest/issues/364
    """

    def __init__(self, get_response: Callable[[HttpRequest], HttpResponse]):
        self.get_response = get_response

    def __call__(self, request: HttpRequest) -> HttpResponse:
        return self.get_response(request)

    def process_exception(self, request: HttpRequest, exception: Exception) -> HttpResponse | None:
        if traceback.extract_tb(exception.__traceback__)[-1].line == "current_fields[segment] = include":
            return HttpResponseBadRequest(
                f"Invalid request with path '{request.get_full_path()}' per message '{exception}'."
            )
        return None  # Indicates to not silence the exception
# settings.py

MIDDLEWARE = [
    ...,
    "cepheus.middleware.DuplicateIncludeExcludeFieldSubstringsMiddleware",
]