Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DjangoResource.as_view should call csrf_exempt #21

Open
selectnull opened this issue Jul 6, 2014 · 4 comments
Open

DjangoResource.as_view should call csrf_exempt #21

selectnull opened this issue Jul 6, 2014 · 4 comments

Comments

@selectnull
Copy link

DjangoResource as_list and as_detail call csrf_exempt on their super, but as_view is not overriden any custom endpoint method is protected with Django CSRF (and it shouldn't be).

@toastdriven
Copy link
Owner

Resolution is in #22, but needs tests before it can be merged.

@schmitch
Copy link
Contributor

I'm still not a fan of this, since I even think that csrf_exempt shouldn't be called.

The OWASP Cheat Sheet is pretty clear about CSRF, too. It's still a big security issue.
Also it's pretty easy to use the Django CSRF token in every Client Based (JavaScript) application. If somebody needs the csrf_exempt he could easily roll out his own "Resource" where he calls csrf_exempt explicitly.

@toastdriven
Copy link
Owner

@c-schmitt I'm confused. For an everyday site leaning on cookies/sessions for web form POSTs, CSRF is essential. But the very nature of an API is that you should be able to access it from other domains/places (think a mobile app) & we're handling authentication explicitly/separately. OWASP's guideline also seem to require performing additional requests in order to get a token.

The typical example (in my head) is that of a mobile app. Pre-OWASP, it looks like:

  • App is fired up & already has valid credentials for the API stored
  • User types in their status-post/todo/picture/whatever & hits send
  • A request is fired to the API (like Restless), which authenticates it & either accepts/denies it

Post-OWASP, I think this looks like:

  • App fires up, with the stored credentials
  • User enters their thing & hits submit
  • BEFORE that data can be sent, the mobile app needs to perform a token request. This would still have to be authed & seems just as fallible, but maybe they get a token (w/ expiry) back if their creds are good.
  • The mobile app must make sure the request succeeded (or potentially retry), make sure they got a valid token back, then can finally send the user's request, merging in the token it just got in the process.
  • The request might fail (like if the process took too long & the token expired, or it was a one-time token but the connection got interrupted), at which point the cycle might need repeating.

The post-OWASP process looks way more complex/fallible (not to mention, Restless has no cross-everything way of storing/generating such tokens), for the gain of shoving authentication to a different place. Perhaps I'm incorrect or overlooking something about this & would love to be corrected on where I'm incorrect.

@selectnull
Copy link
Author

I must admit confusion on my part as well as I agree with @toastdriven's description of how APIs should work, so after thinking and googling a bit I would like to present my findings, hopefully be less confused and initiate more conversation. These are few links that I found relevant:

  • CSRF prevention for RESTful services
    • "Cookies are automatically included in the payload from the browser to the server for the domain being contacted. Therefore that communication is susceptible to csrf."
  • django-rest-framework on CSRF protection
    • "To guard against... Ensure that any 'unsafe' HTTP operations, such as POST, PUT, PATCH and DELETE, always require a valid CSRF token."
  • OWASP REST Security Cheat Sheet mentiond by @c-schmitt
    • definitely advises in favor of having CSRF protection in APIs
  • Post in django-developers group by Tom Christie from 2012 Making sure Web APIs built with Django don't have CSRF vulnerabilities
    • "Most of the time when building Web APIs with Django the right thing to do is to wrap API views in @csrf_exempt. Generally Web APIs shouldn't have CSRF, since API clients authenticate their requests explicitly, and don't need (or have) a CSRF token available to them. That's the right thing to do except for the case where you're building a Web API that is consumed by an AJAX client of the same site. In that case the AJAX call happens within the context of the current session, and the authentication is implicit in the request."

It appears that people are of the opinion that APIs, restfull or not, should be protected from CSRF.

Initially, my thoughts on the topic were that any API resource should be csrf exempt and therefor my pull request 306c8fb. One of the reasons was similarity with as_list and as_detail methods. The reason for not wanting protection from CSRF is that I am using JWT for auth and don't need another token (which csrf_token in that scenario effectively is, just one more token along side jwt one).

Now, I'm leaning towards agreeing with @c-schmitt. Frameworks should not make the decision if API views should or shouldn't be csrf exempt. There are APIs that use cookie based auth and those are suspectible to CSRF. My conlusion is that if an application is using Django and is configured to be protected against CSRF then framework such as restless should not use csrf_exempt. If the developer uses JWT auth only (and not django cookie based auth), they should remove CsrfViewMiddleware from settings but that choice must not be decided in advance by the framework for him/her.

I see two possible ways this can be resolved:

  1. we agree that Call csrf_exempt on DjangoResource.as_view. #22 is good for restless and I write those tests needed
  2. we agree that csrf_exempt should be removed from as_list and as_detail (in dj.py)

I still want to write those tests for #22 and plan to do so in the following weeks as those will be useful in any case this goes forward. I would definitely want to continue this discussion in the meantime and hear your opinions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants