Project

Profile

Help

Issue #3906

browsable API inserts a csrf token field into all form submissions

Added by dkliban@redhat.com about 1 year ago. Updated 6 months ago.

Status:
MODIFIED
Priority:
Normal
Assignee:
Category:
-
Sprint/Milestone:
Start date:
Due date:
Severity:
2. Medium
Version:
Platform Release:
Blocks Release:
OS:
Backwards Incompatible:
No
Triaged:
Yes
Groomed:
No
Sprint Candidate:
No
Tags:
QA Contact:
Complexity:
Smash Test:
Verified:
No
Verification Required:
No
Sprint:
Sprint 41

Description

Pulp's REST API validates that only acceptable fields are submitted with each request. The list of fields does not include the csrf token. As a result of this validation, the browsable API forms produce responses that look like this:

    HTTP 400 Bad Request
    Allow: GET, POST, HEAD, OPTIONS
    Content-Type: application/json
    Vary: Accept

    {
        "csrfmiddlewaretoken": [
            "Unexpected field" 
        ]
    }

We need to investigate how to configure DRF to stop including this field with each form. Otherwise a fix from comment 3 would be appropriate.

Notes

To easily reproduce, navigate your browser to e.g http://localhost:8000/pulp/api/v3/repositories/ and create a repository by filling-in the create repository form.
This happens when session authentication is used such as with a browser; non-session authentication won't trigger the CSRF protection.
Unfortunately it seems the session-authentication is somehow mandatory or at least I wasn't able to just switch it off:

pulp: django.request:ERROR: Internal Server Error: /pulp/api/v3/repositories/
Traceback (most recent call last):
  File "/home/vagrant/.virtualenvs/pulp/lib64/python3.6/site-packages/django/core/handlers/exception.py", line 34, in inner
    response = get_response(request)
  File "/home/vagrant/.virtualenvs/pulp/lib64/python3.6/site-packages/django/utils/deprecation.py", line 90, in __call__
    response = self.process_request(request)
  File "/home/vagrant/.virtualenvs/pulp/lib64/python3.6/site-packages/django/contrib/auth/middleware.py", line 23, in process_request
    ) % ("_CLASSES" if settings.MIDDLEWARE is None else "")
AssertionError: The Django authentication middleware requires session middleware to be installed. Edit your MIDDLEWARE setting to insert 'django.contrib.sessions.middleware.SessionMiddleware' before 'django.contrib.auth.middleware.AuthenticationMiddleware'.
[13/Aug/2018 13:15:14] "GET /pulp/api/v3/repositories/ HTTP/1.1" 500 69693

Discussion

The session authentication is configured for particular Django application in app/settings.py:

MIDDLEWARE = [
  ...
  django.middleware.csrf.CsrfViewMiddleware,
  django.contrib.auth.middleware.AuthenticationMiddleware,
]
...

REST_FRAMEWORK = {
  ...
  'DEFAULT_AUTHENTICATION_CLASSES': [
    rest_framework.authentication.SessionAuthentication,
    rest_framework.authentication.BasicAuthentication,
  ]
}

DRF uses a custom authentication class SessionAuthentication that in turn utilizes the Django middleware CsrfViewMiddleware class thru the CSRFCheck class, so the CsrfViewMiddleware:process_view gets eventually triggered. The request_csrf_token is then extracted from the request.POST.get('csrfmiddlewaretoken') form field, which is unset neither in the Django nor in the DRF middleware classes and the csrfmiddlewaretoken form field gets eventually propagated into a Pulp create object view.

Good news is we should be fine to just ignore it; "greping" over Github, this particular csrfmiddlewaretoken string shows quite often in the code so often in fact that I can't confirm some of it being DRF/Django workarounds.


Related issues

Related to Pulp - Issue #2970: REST API silently ignores object attributes that don't exist on the serializer MODIFIED Actions

Associated revisions

Revision 7ce37c36 View on GitHub
Added by milan about 1 year ago

Serializer ignore the csrfmiddlewaretoken field

The CSRF protection middleware kicks-in when a browser is used to access
Pulp API, after the user having successfully logged into Pulp, i.e
whenever a Session-based authentication is used by the DRF/Django
The middleware uses a custom (and invisible) form field:
`csrfmiddlewaretoken` that the page in the browser populates from a CSRF
cookie previously generated by Django whenever issuing a
POST/PATCH/DELETE... request to prevent the cross-site-request-forgery.
The whole mechanism is nicely described on wikipedia.

This `csrfmiddlewaretoken` field unfortunately isn't stripped from the
POSTed data by either DRF or Django, and causes our serializers to fail
validate form data because of it being an unknown field to our models.

This patch just makes our validation ignore the unknow
`csrfmiddlewaretoken` field.

Fixes: #3906
https://pulp.plan.io/issues/3906

Revision 7ce37c36 View on GitHub
Added by milan about 1 year ago

Serializer ignore the csrfmiddlewaretoken field

The CSRF protection middleware kicks-in when a browser is used to access
Pulp API, after the user having successfully logged into Pulp, i.e
whenever a Session-based authentication is used by the DRF/Django
The middleware uses a custom (and invisible) form field:
`csrfmiddlewaretoken` that the page in the browser populates from a CSRF
cookie previously generated by Django whenever issuing a
POST/PATCH/DELETE... request to prevent the cross-site-request-forgery.
The whole mechanism is nicely described on wikipedia.

This `csrfmiddlewaretoken` field unfortunately isn't stripped from the
POSTed data by either DRF or Django, and causes our serializers to fail
validate form data because of it being an unknown field to our models.

This patch just makes our validation ignore the unknow
`csrfmiddlewaretoken` field.

Fixes: #3906
https://pulp.plan.io/issues/3906

Revision 7ce37c36 View on GitHub
Added by milan about 1 year ago

Serializer ignore the csrfmiddlewaretoken field

The CSRF protection middleware kicks-in when a browser is used to access
Pulp API, after the user having successfully logged into Pulp, i.e
whenever a Session-based authentication is used by the DRF/Django
The middleware uses a custom (and invisible) form field:
`csrfmiddlewaretoken` that the page in the browser populates from a CSRF
cookie previously generated by Django whenever issuing a
POST/PATCH/DELETE... request to prevent the cross-site-request-forgery.
The whole mechanism is nicely described on wikipedia.

This `csrfmiddlewaretoken` field unfortunately isn't stripped from the
POSTed data by either DRF or Django, and causes our serializers to fail
validate form data because of it being an unknown field to our models.

This patch just makes our validation ignore the unknow
`csrfmiddlewaretoken` field.

Fixes: #3906
https://pulp.plan.io/issues/3906

History

#1 Updated by daviddavis about 1 year ago

  • Related to Issue #2970: REST API silently ignores object attributes that don't exist on the serializer added

#2 Updated by dkliban@redhat.com about 1 year ago

  • Subject changed from csrf middleware breaks browsable API to browsable API inserts a csrf token field into all form submissions
  • Description updated (diff)

#3 Updated by daviddavis about 1 year ago

One possible solution is to update our unknown field validator to ignore the field:

diff --git a/pulpcore/pulpcore/app/serializers/base.py b/pulpcore/pulpcore/app/serializers/base.py
index 2a1aef1..66410dc 100644
--- a/pulpcore/pulpcore/app/serializers/base.py
+++ b/pulpcore/pulpcore/app/serializers/base.py
@@ -81,7 +81,8 @@ def validate_unknown_fields(initial_data, defined_fields):
     """ 
     This will raise a `ValidationError` if a serializer is passed fields that are unknown.
     """ 
-    unknown_fields = set(initial_data) - set(defined_fields)
+    ignored_fields = {'csrfmiddlewaretoken'}
+    unknown_fields = set(initial_data) - set(defined_fields) - ignored_fields
     if unknown_fields:
         unknown_fields = {field: _('Unexpected field') for field in unknown_fields}
         raise serializers.ValidationError(unknown_fields)

#4 Updated by dkliban@redhat.com about 1 year ago

  • Description updated (diff)

#5 Updated by dkliban@redhat.com about 1 year ago

  • Description updated (diff)

#6 Updated by dkliban@redhat.com about 1 year ago

  • Description updated (diff)

#7 Updated by CodeHeeler about 1 year ago

  • Triaged changed from No to Yes
  • Sprint set to Sprint 41

#8 Updated by milan about 1 year ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to milan

#9 Updated by milan about 1 year ago

  • Description updated (diff)

#10 Updated by milan about 1 year ago

  • Description updated (diff)

#11 Updated by milan about 1 year ago

  • Description updated (diff)

#12 Updated by milan about 1 year ago

When trying to address this in DRF (authentication classes) by stripping the csrfmiddlewaretoken from the request POST object I get this error:

___________________________________________________________ NoDropdownWithoutAuthTests.test_name_shown_when_logged_in ____________________________________________________________
tests/browsable_api/test_browsable_api.py:63: in test_name_shown_when_logged_in
    response = self.client.get('/')
rest_framework/test.py:292: in get
    response = super(APIClient, self).get(path, data=data, **extra)
rest_framework/test.py:209: in get
    return self.generic('GET', path, **r)
rest_framework/test.py:238: in generic
    method, path, data, content_type, secure, **extra)
env/lib/python2.7/site-packages/django/test/client.py:416: in generic
    return self.request(**r)
rest_framework/test.py:289: in request
    return super(APIClient, self).request(**kwargs)
rest_framework/test.py:241: in request
    request = super(APIRequestFactory, self).request(**kwargs)
env/lib/python2.7/site-packages/django/test/client.py:483: in request
    response = self.handler(environ)
env/lib/python2.7/site-packages/django/test/client.py:144: in __call__
    response = self.get_response(request)
rest_framework/test.py:261: in get_response
    return super(ForceAuthClientHandler, self).get_response(request)
env/lib/python2.7/site-packages/django/core/handlers/base.py:124: in get_response
    response = self._middleware_chain(request)
env/lib/python2.7/site-packages/django/core/handlers/exception.py:43: in inner
    response = response_for_exception(request, exc)
env/lib/python2.7/site-packages/django/core/handlers/exception.py:93: in response_for_exception
    response = handle_uncaught_exception(request, get_resolver(get_urlconf()), sys.exc_info())
env/lib/python2.7/site-packages/django/core/handlers/exception.py:41: in inner
    response = get_response(request)
env/lib/python2.7/site-packages/django/utils/deprecation.py:140: in __call__
    response = self.get_response(request)
env/lib/python2.7/site-packages/django/core/handlers/exception.py:43: in inner
    response = response_for_exception(request, exc)
env/lib/python2.7/site-packages/django/core/handlers/exception.py:93: in response_for_exception
    response = handle_uncaught_exception(request, get_resolver(get_urlconf()), sys.exc_info())
env/lib/python2.7/site-packages/django/core/handlers/exception.py:41: in inner
    response = get_response(request)
env/lib/python2.7/site-packages/django/utils/deprecation.py:140: in __call__
    response = self.get_response(request)
env/lib/python2.7/site-packages/django/core/handlers/exception.py:43: in inner
    response = response_for_exception(request, exc)
env/lib/python2.7/site-packages/django/core/handlers/exception.py:93: in response_for_exception
    response = handle_uncaught_exception(request, get_resolver(get_urlconf()), sys.exc_info())
env/lib/python2.7/site-packages/django/core/handlers/exception.py:41: in inner
    response = get_response(request)
env/lib/python2.7/site-packages/django/utils/deprecation.py:140: in __call__
    response = self.get_response(request)
env/lib/python2.7/site-packages/django/core/handlers/exception.py:43: in inner
    response = response_for_exception(request, exc)
env/lib/python2.7/site-packages/django/core/handlers/exception.py:93: in response_for_exception
    response = handle_uncaught_exception(request, get_resolver(get_urlconf()), sys.exc_info())
env/lib/python2.7/site-packages/django/core/handlers/exception.py:41: in inner
    response = get_response(request)
env/lib/python2.7/site-packages/django/utils/deprecation.py:140: in __call__
    response = self.get_response(request)
env/lib/python2.7/site-packages/django/core/handlers/exception.py:43: in inner
    response = response_for_exception(request, exc)
env/lib/python2.7/site-packages/django/core/handlers/exception.py:93: in response_for_exception
    response = handle_uncaught_exception(request, get_resolver(get_urlconf()), sys.exc_info())
env/lib/python2.7/site-packages/django/core/handlers/exception.py:41: in inner
    response = get_response(request)
env/lib/python2.7/site-packages/django/core/handlers/base.py:187: in _get_response
    response = self.process_exception_by_middleware(e, request)
env/lib/python2.7/site-packages/django/core/handlers/base.py:185: in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
env/lib/python2.7/site-packages/django/views/decorators/csrf.py:58: in wrapped_view
    return view_func(*args, **kwargs)
env/lib/python2.7/site-packages/django/views/generic/base.py:68: in view
    return self.dispatch(request, *args, **kwargs)
rest_framework/views.py:495: in dispatch
    response = self.handle_exception(exc)
rest_framework/views.py:455: in handle_exception
    self.raise_uncaught_exception(exc)
rest_framework/views.py:483: in dispatch
    self.initial(request, *args, **kwargs)
rest_framework/views.py:400: in initial
    self.perform_authentication(request)
rest_framework/views.py:326: in perform_authentication
    request.user
rest_framework/request.py:222: in user
    self._authenticate()
/usr/lib64/python2.7/contextlib.py:35: in __exit__
    self.gen.throw(type, value, traceback)
rest_framework/request.py:80: in wrap_attributeerrors
    six.reraise(type(exc), exc, info[2])
rest_framework/request.py:76: in wrap_attributeerrors
    yield
rest_framework/request.py:222: in user
    self._authenticate()
rest_framework/request.py:375: in _authenticate
    user_auth_tuple = authenticator.authenticate(self)
rest_framework/authentication.py:129: in authenticate
    self.enforce_csrf(request)
rest_framework/authentication.py:148: in enforce_csrf
    del(request.POST['csrfmiddlewaretoken'])
env/lib/python2.7/site-packages/django/http/request.py:440: in __delitem__
    self._assert_mutable()
env/lib/python2.7/site-packages/django/http/request.py:431: in _assert_mutable
    raise AttributeError("This QueryDict instance is immutable")
E   WrappedAttributeError: This QueryDict instance is immutable

Which makes me think that the request data shouldn't be touched (in the outside of the middleware?)
So I guess the simplest thing to do is to treat it as @daviddavis suggests: ignore

#13 Updated by daviddavis about 1 year ago

+1 to just ignoring the param in validate_unknown_fields.

#14 Updated by milan about 1 year ago

  • Status changed from ASSIGNED to POST

#15 Updated by milan about 1 year ago

I've done some more investigation and I guess this is actually some DRF bug; see this call stack from my PDB session:

  /home/vagrant/devel/django/django/core/handlers/base.py(124)_get_response()
-> response = wrapped_callback(request, *callback_args, **callback_kwargs)
  /home/vagrant/devel/django/django/views/decorators/csrf.py(54)wrapped_view()
-> return view_func(*args, **kwargs)
> /home/vagrant/.virtualenvs/pulp/lib64/python3.6/site-packages/rest_framework/viewsets.py(82)view()

....

> /home/vagrant/.virtualenvs/pulp/lib64/python3.6/site-packages/rest_framework/viewsets.py(103)view()-><Response sta...harset=utf-8">
-> return self.dispatch(request, *args, **kwargs)
(Pdb) list
 98                 self.request = request
 99                 self.args = args
100                 self.kwargs = kwargs
101
102                 # And continue as usual
103  ->             return self.dispatch(request, *args, **kwargs)
104
105             # take name and docstring from class
106             update_wrapper(view, cls, updated=())
107
108             # and possible attributes set by decorators
(Pdb) next
--Return--
> /home/vagrant/devel/django/django/views/decorators/csrf.py(54)wrapped_view()-><Response sta...harset=utf-8">
-> return view_func(*args, **kwargs)
(Pdb) list
 49     def csrf_exempt(view_func):
 50         """Mark a view function as being exempt from the CSRF view protection.""" 
 51         # view_func.csrf_exempt = True would also work, but decorators are nicer
 52         # if they don't have side effects, so return a new function.
 53         def wrapped_view(*args, **kwargs):
 54  ->         return view_func(*args, **kwargs)
 55         wrapped_view.csrf_exempt = True
 56         return wraps(view_func)(wrapped_view)

So it seems the csrf_exempt is actually always applied to our views.

#16 Updated by milan about 1 year ago

  • Status changed from POST to MODIFIED

#17 Updated by bmbouter 6 months ago

  • Tags deleted (Pulp 3)

Please register to edit this issue

Also available in: Atom PDF